refactor: replaced _implicitHeader

[lamweili]

Fixes #888

Replaced the usage of undocumented HTTP API with its implementation instead (bringing about some* http2 support).
There is no change to the existing behavior. This is similar to expressjs/compression#170.

• Undocumented res._implicitHeader https://github.com/nodejs/node/blob/a8bc202d855cc7d14895db38a2ac09d1f873e803/lib/_http_server.js#L281
  Replaced with res._implicitHeader implementation
  https://github.com/nodejs/node/blob/a8bc202d855cc7d14895db38a2ac09d1f873e803/lib/_http_server.js#L282

• Undocumented res._header
  https://github.com/nodejs/node/blob/93e0bf9abf350637d772bcce14a5d9527b733300/lib/_http_outgoing.js#L741
  Replaced with res.headersSent
  https://github.com/nodejs/node/blob/93e0bf9abf350637d772bcce14a5d9527b733300/lib/_http_outgoing.js#L745

The changes do not require a semver major.

some* - This change may/may not provide full http2 support.

[dougwilson]

I'm not sure what the purpose of the added test code is in relation to this change, as (1) reverting the changes in index.js doesn't affect the outcome of the tests and (2) looking at the test code it is just a http/2 server returning "Hello, world!" and asserting that that is the case...

[dougwilson]

Sweet. Also, I am going to sleep now, but meant to update here that I think I was using a Node.js version without http/2 earlier when I tested; I needed to get back and double-check that.

[lamweili]

Ok, I have split the initial commit (d021958) into two separate commits (92d05cc, df7163e) for clarity.

1. 92d05cc - added test case which fails if res._implicitHeader() is used (e.g. in http2)
   I added tests that actually require this change, such that we don't accidentally regress the behavior.
   The existing codes are untouched. Checking out to it, we will get a test failure on existing codes.

2. df7163e - replaced res._implicitHeader() to address test case
   In the next commit, which replaced the res._implicitHeader(), the test will now pass. As we are using the implementation of res._implicitHeader(), semver major is not required.

3. 416ae6f - removed explicit http2 headers for less verbosity (using default values)

I ran the GH actions on my forked repository and here are the results for all the commits:
https://github.com/lamweili/session/actions

[lamweili]

I'm not sure if this change will provide full http2 support.

IMHO, changing from an undocumented API to a documented API is a move in the right direction.
As a by-product, it helps to remove a blocker in the http2 scope, as http2 does not have res._implicitHeader().

@dougwilson, do you think we should add a repeat of all the tests for http2?
Or should we put that aside for now and simply treat this PR as a refactor of undocumented API to documented API?

[dougwilson]

Yep, makes sense. I assume the line right above that should be changed as well, right? res._header is also an undocumented HTTP API, which seems to be the purpose of the PR based on the description. We really should just only use public APIs for sure 👍 . A lot of this is just kruft, haha

[lamweili]

Yup, agreed.
For clarity, PR only fixes ONE undocumented API to documented API, which is res._implicitHeader(). 🤗

[dougwilson]

Gotcha. Why spread it across a bunch of PRs? We can just fix the undocumented API usage at once. It's better for making a release, as making a release can cause folks to break. It is sad but true. I usually err on this being a semver major so we should get them all fixed at once.

[dougwilson]

I'm not sure what that means. It is used in the line right above the one you changed in the PR.

session/index.js

Line 275 in 1010fad

if (!res._header) {

[lamweili]

Ah, I don't seem to find any res._header.
I guess we got lucky?

Ah, my bad, I found it. Bad search I had previously.

I will address the res._header as well then. Any others to take note of?

session/index.js

Line 275 in 1010fad

if (!res._header) {

[dougwilson]

I'm on mobile, so not sure. I just saw that one right above the line you changed when looking at the diff. I can help take a look over the code to find them tomorrow 👍

[lamweili]

Continuation of #908 (comment):

4. 35880d6 - added tests to check for res._header and res.headersSent equivalence
   Strangely, while res.headersSent is only available from Node.js v0.9.4, all test cases (including the added one) passed.
   Perhaps the documentation is incorrect?

5. 36618af - replaced res._header with res.headersSent if available (prevented regression so no need for semver major)

I ran the GH actions on my forked repository and here are the results for all the commits:
https://github.com/lamweili/session/actions

[lamweili]

@dougwilson, have you got the time to take a look at this PR?

[Martin-Luther]

I had the same issue : http2 + express-session and the latest version of express-session 1.17.3 did not help.
express-session is not the only module impacted: I saw people having the same issue with express-compression.

NB.: I am not using ExpressJS, but my framework allows me to use modules/plugins from ExpressJS

Option 1)
You don't have the time to wait for a fix, you can intercept your response and add a test inside your script right before calling res.end(...)

if (!res._implicitHeader) {
    // Hack required until `express-<plugin>` get support for http2 (express-session)
    res._implicitHeader = function(){ return; }; // Don't need, don't care
}
res.end();

This worked for me.

Option 2)
For those brave enough to fixe this issue for people like me.
Open express-session/index.js:275

You should have something like:

if (!res._header) {
    res._implicitHeader()
}

Needs to be replaced by:

if (!res._header && res._implicitHeader) {
    res._implicitHeader()
}

This will skip the call for people using http2 or when res._implicitHeader not defined.

Let me know if it worked for you guys.

[lamweili]

@dougwilson Not sure if you have the time to have a look at this PR again?