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 https://github.com/expressjs/compression/pull/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]

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. https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L275

[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? https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L275

[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]

Sure, thanks! While I have your attention, can you also kindly review https://github.com/expressjs/compression/pull/170? (I will delete this comment thereafter, just needed the GitHub to notify you.)

[dougwilson]

Yes, I will. I only had so much time today for open source. I was just doing them from top to bottom in the GH notifications. I haven't marked that notification as done yet, but also cannot promise that when I get back to it tomorrow, I'll look at that, esp. if you have new stuff on this one. I just go from top to bottom in my GH notifications with whatever time I have is all I can promise.

[lamweili]

Continuation of https://github.com/expressjs/session/pull/908#issuecomment-1203407814:

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?