refactor: replaced _implicitHeader #908

lamweili · 2022-08-02T07:49:17Z

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...

test/session.js

dougwilson · 2022-08-03T01:45:26Z

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.

…mplicitHeader()`

… method (bringing about http2 support) - https://github.com/nodejs/node/blob/a8bc202d855cc7d14895db38a2ac09d1f873e803/lib/_http_server.js#L281 - https://github.com/nodejs/node/blob/a8bc202d855cc7d14895db38a2ac09d1f873e803/lib/_http_server.js#L282

lamweili · 2022-08-03T02:20:58Z

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

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.
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.
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 · 2022-08-03T02:22:17Z

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 · 2022-08-03T02:34:32Z

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 · 2022-08-03T02:42:03Z

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

dougwilson · 2022-08-03T02:44:02Z

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 · 2022-08-03T02:47:35Z

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 · 2022-08-03T02:47:49Z

~~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 · 2022-08-03T02:48:37Z

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 👍

…ivalence

…t` (regression-safe) - https://github.com/nodejs/node/blob/93e0bf9abf350637d772bcce14a5d9527b733300/lib/_http_outgoing.js#L741-L746

lamweili · 2022-08-04T02:39:44Z

Continuation of #908 (comment):

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?
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 · 2022-10-07T16:21:21Z

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

Martin-Luther · 2022-12-10T02:55:58Z

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 · 2023-04-22T13:59:20Z

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

bjohansebas · 2024-11-29T00:00:19Z

index.js

@@ -272,8 +272,8 @@ function session(options) {
          return ret;
        }

-        if (!res._header) {
-          res._implicitHeader()
+        if (typeof res.headersSent === 'boolean' ? !res.headersSent : !res._header) {


Maybe it's better to make it a function like in compression expressjs/compression#129

bjohansebas · 2024-11-29T00:01:27Z

test/session.js

@@ -1371,6 +1381,25 @@ describe('session()', function(){
    })
  });

+  describe('res._header patch', function () {


I don't think this test makes sense.

lamweili mentioned this pull request Aug 2, 2022

feat: add support for http2 #900

Closed

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch 4 times, most recently from d2392b6 to 12eb09c Compare August 2, 2022 09:38

import-brain added the pr label Aug 2, 2022

dougwilson suggested changes Aug 2, 2022

View reviewed changes

test/session.js Outdated Show resolved Hide resolved

dougwilson added the needs tests label Aug 2, 2022

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch 2 times, most recently from cf9cc21 to 4d16065 Compare August 3, 2022 02:06

lamweili added 2 commits August 3, 2022 10:10

Added test cases (http2) that will fail if using undocumented `res._i…

92d05cc

…mplicitHeader()`

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch from cf9cc21 to 92d05cc Compare August 3, 2022 02:10

lamweili requested a review from dougwilson August 3, 2022 02:31

Removed explicit http2 headers for less verbosity (using default values)

416ae6f

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch from 6770cbf to 416ae6f Compare August 3, 2022 02:37

This comment was marked as off-topic.

Sign in to view

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch 3 times, most recently from 647bdd1 to 9e240a5 Compare August 3, 2022 12:16

Added test cases to check for res._header and res.headersSent equ…

35880d6

…ivalence

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch from e7b53e3 to 35880d6 Compare August 3, 2022 16:00

Replaced the usage of undocumented res._header with `res.headersSen…

36618af

…t` (regression-safe) - https://github.com/nodejs/node/blob/93e0bf9abf350637d772bcce14a5d9527b733300/lib/_http_outgoing.js#L741-L746

lamweili force-pushed the refactor/use_writeHead_instead_of_implicitHeader branch from 7b8de6b to 36618af Compare August 4, 2022 02:38

dougwilson force-pushed the master branch 2 times, most recently from 9d2e29b to 408229e Compare January 28, 2024 20:24

bjohansebas reviewed Nov 29, 2024

View reviewed changes

bjohansebas mentioned this pull request Dec 6, 2024

backlog for session@2.x #1006

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replaced _implicitHeader #908

refactor: replaced _implicitHeader #908

lamweili commented Aug 2, 2022 •

edited

Loading

dougwilson left a comment

dougwilson commented Aug 3, 2022

lamweili commented Aug 3, 2022 •

edited

Loading

lamweili commented Aug 3, 2022 •

edited

Loading

dougwilson commented Aug 3, 2022

lamweili commented Aug 3, 2022 •

edited

Loading

dougwilson commented Aug 3, 2022

dougwilson commented Aug 3, 2022

lamweili commented Aug 3, 2022 •

edited

Loading

dougwilson commented Aug 3, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

lamweili commented Aug 4, 2022

lamweili commented Oct 7, 2022

Martin-Luther commented Dec 10, 2022 •

edited

Loading

lamweili commented Apr 22, 2023 •

edited

Loading

bjohansebas Nov 29, 2024

bjohansebas Nov 29, 2024

refactor: replaced _implicitHeader #908

Are you sure you want to change the base?

refactor: replaced _implicitHeader #908

Conversation

lamweili commented Aug 2, 2022 • edited Loading

dougwilson left a comment

Choose a reason for hiding this comment

dougwilson commented Aug 3, 2022

lamweili commented Aug 3, 2022 • edited Loading

lamweili commented Aug 3, 2022 • edited Loading

dougwilson commented Aug 3, 2022

lamweili commented Aug 3, 2022 • edited Loading

dougwilson commented Aug 3, 2022

dougwilson commented Aug 3, 2022

lamweili commented Aug 3, 2022 • edited Loading

dougwilson commented Aug 3, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

lamweili commented Aug 4, 2022

lamweili commented Oct 7, 2022

Martin-Luther commented Dec 10, 2022 • edited Loading

lamweili commented Apr 22, 2023 • edited Loading

bjohansebas Nov 29, 2024

Choose a reason for hiding this comment

bjohansebas Nov 29, 2024

Choose a reason for hiding this comment

lamweili commented Aug 2, 2022 •

edited

Loading

lamweili commented Aug 3, 2022 •

edited

Loading

lamweili commented Aug 3, 2022 •

edited

Loading

lamweili commented Aug 3, 2022 •

edited

Loading

lamweili commented Aug 3, 2022 •

edited

Loading

Martin-Luther commented Dec 10, 2022 •

edited

Loading

lamweili commented Apr 22, 2023 •

edited

Loading