Bugfix/use write head instead of implicit header by Icehunter · Pull Request #170 · expressjs/compression · GitHub

updated PR with rebase from express/compress:master to handle #128

Just wanted to note on here it looks like just a rebase, without addressing the review comments yet. I presume that will come in follow up commits?

Just wanted to note on here it looks like just a rebase, without addressing the review comments yet. I presume that will come in follow up commits?

Yes, working on that now.

@dougwilson I found all the comments I think needed to be fixed, except 2 perhaps that I'll work on tonight.

the on('data') callback which I think doesn't need a data check; per the noop it's just a way to prevent hangs
the comments from @sogaani regarding the response code. I'll look at that as well.

@dougwilson Is there anything more you see for this PR to have?
@sogaani with regards to your comment on the error status code; it seems to be a big edge case. I work on a very large API and haven't encountered this issue before.

I was under the impression you were going to make changes in regards to the two bullet points above, which is what I'm waiting on. I never saw any additional comments after that message. Are you still working on them?

Eh! Sorry; I did leave the on('data') callback as is in the test, but I removed all the logs and I believe covered everything you had commented on in the previous test.

For the second bullet point I quite literally have no clue off the top of my head how to even cover that in an edge case test.

Seems like this one got stuck right?

Hi @Icehunter and @dougwilson,

IMHO, there are two issues covered by this PR.

Switching from an undocumented API to a public API.
This is definitely preferred. Current tests should suffice as there are no logical changes.
Supporting http2 as a by-product.
Additional tests required to prove support on http2.

I addressed the on('data') callback as mentioned in #128 (comment) and #170 (comment) with the following suggestions.
It now asserts that the data is correct.

Go to the Files tab and Add suggestion to batch so that it becomes a singular commit.
You might also want to correct the indentations within the function closeHttp2 at line 733.

I believe with this, all outstanding review comments in #128 are addressed and this PR should be ready and supersede #128.

@Icehunter, you might want to correct the indentations at line 740-744 and line 747-753.
I didn't include them previously so that 38fe9fd is surgical.

compression/test/compression.js

Lines 737 to 756 in 38fe9fd

    
           function closeHttp2 (client, server, callback) { 
        
             if (typeof client.shutdown === 'function') { 
        
               // this is the node v8.x way of closing the connections 
        
                 client.shutdown({}, function () { 
        
                   server.close(function () { 
        
                     callback() 
        
                   }) 
        
                 }) 
        
             } else { 
        
               // this is the node v9.x onwards way of closing the connections 
        
                 client.close(function () { 
        
                   // force existing connections to time out after 1ms. 
        
                   // this is done to force the server to close in some cases where it wouldn't do it otherwise. 
        
                   server.close(function () { 
        
                     callback() 
        
                   }) 
        
                 }) 
        
             } 
        
           }

@Icehunter, for the clarity that #129 is not in this PR, the description should change:

updated PR with rebase from express/compress:master to handle #128 and #129

The change just removes the usage of undocumented http API and instead uses the proper writeHead method

… in node v10.4 onwards

Co-authored-by: Lam Wei Li <lam_wei_li@hotmail.com>

@Icehunter, for the clarity that #129 is not in this PR, the description should change:

updated PR with rebase from express/compress:master to handle #128 and #129

Updated and rebased again :)

@dougwilson When you are free, can you review this PR?
Fixes #122 and supersedes #128.
Many thanks! 🤗

I didn't have access in the forked repository to run the GH actions. So I forked the PR.
I ran the GH actions on my own repository and here is the result for the latest commit (4c1d27b):
https://github.com/lamweili/compression/actions

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

LGTM

(can't approve cause I don't have explicit access to this repository)
😮‍💨

…ader

sorry @Icehunter, Can you fix the code style?

sorry @Icehunter, Can you fix the code style?

Sure thing.

Dropping some context from slack re: this PR

The original issue here is compression relying on node http internals since the days of connect in 2011

Here's when the commit w/ _implicitHeader originally landed:

senchalabs/connect@715e4f5

http2 module has a different API and internals than http1 in node. So that method doesn't exist at all in http2, throwing when used.

Compression module doesn't support http2 explicitly. It likely can, but was never intended to.
writeHead has been around since that first commit, but for reasons lost to time wasn't used

Idk that I can say that issue #122 "supporting http2" is closed after landing #170, because I personally haven't reviewed the code or tested for http2 edgecases.

However, removing an http internal to replace w/ a public interface which is equivalent is 💯

It doesn't look like _implicitHeader implementation has changed much since Node v0.4.12:
https://github.com/nodejs/node/blob/v0.4.12/lib/http.js#L796

Here it is in v23:
https://github.com/nodejs/node/blob/v23.1.0/lib/_http_server.js#L336

This history is long w/ 19 commits, @bjohansebas when/if you merge I'd suggest squashing via the dropdown to keep the history clean

history.md isn't updated in this PR, we've talked about dropping that file (at least blake is planning to do so) but just a reminder in case you want to make sure you don't forget changelog update

dougwilson added the pr label Jul 10, 2020

lamweili suggested changes Jul 30, 2022

View reviewed changes

Moritz Peters and others added 13 commits July 30, 2022 12:33

Add failing tests for http2

f160f44

Add support for http2

2c8ed82

The change just removes the usage of undocumented http API and instead uses the proper writeHead method

Remove arrow function in tests

8eae006

Conditionally run http2 tests if http2 is available

8c68bc1

Fix port in tests to be assigned automatically

5122916

Change http2 test usage to describe.skip if no http2 available

7b7f506

Fix closing the http2 connections to prevent possible exceptions

8ba8100

Fix closing the request first, then the client, then the server

8ea806d

Fix closing for v8.x and v9.x

a562056

fix tests not draining data for http2 requests, resulting in timeouts…

feddec1

… in node v10.4 onwards

fix: 🐛 assert.equal error

5124eb0

fix: 🐛 remove console.log's and timeout, let build fail

094316e

Apply suggestions from code review

d85e6e8

Co-authored-by: Lam Wei Li <lam_wei_li@hotmail.com>

Icehunter force-pushed the bugfix/use_writeHead_instead_of_implicitHeader branch from 38fe9fd to d85e6e8 Compare July 30, 2022 19:33

lamweili force-pushed the bugfix/use_writeHead_instead_of_implicitHeader branch from 0dafcad to b908af0 Compare August 2, 2022 07:36

This was referenced Aug 2, 2022

Fix usage of undocumented _implicitHeader #128

Closed

refactor: replaced _implicitHeader expressjs/session#908

Closed

lamweili force-pushed the bugfix/use_writeHead_instead_of_implicitHeader branch from 4d77342 to b908af0 Compare August 2, 2022 08:37

Node.js 0.8 compatible

4c1d27b

lamweili force-pushed the bugfix/use_writeHead_instead_of_implicitHeader branch from b908af0 to 4c1d27b Compare August 2, 2022 09:49

jordanst3wart mentioned this pull request Sep 25, 2023

code samples, an interface, or middleware for fastify (express is becoming legacy) auth0/express-openid-connect#504

Closed

5 tasks

bjohansebas mentioned this pull request Sep 20, 2024

Release 1.8.0 #189

Closed

bjohansebas added this to the 1.8.0 milestone Nov 3, 2024

bjohansebas reviewed Nov 3, 2024

View reviewed changes

Comment thread test/compression.js

bjohansebas requested review from a team and UlisesGascon November 4, 2024 00:16

bjohansebas approved these changes Nov 4, 2024

View reviewed changes

bjohansebas self-assigned this Nov 4, 2024

bjohansebas added enhancement and removed pr labels Nov 4, 2024

Icehunter requested a review from lamweili November 4, 2024 00:31

lamweili reviewed Nov 4, 2024

View reviewed changes

Merge branch 'master' into bugfix/use_writeHead_instead_of_implicitHe…

ec797a7

…ader

fix lint issue

c126f78

Icehunter requested a review from lamweili November 9, 2024 18:10

jonchurch mentioned this pull request Nov 10, 2024

Support for Node.js 8 native http2 #122

Open

jonchurch approved these changes Nov 10, 2024

View reviewed changes

bjohansebas merged commit b7d5d77 into expressjs:master Nov 13, 2024

bjohansebas mentioned this pull request Jan 8, 2025

release: 1.8.0 #212

Merged

nobodysfool mentioned this pull request Jul 18, 2025

Bump on-headers and compression userfrosting/UserFrosting#1291

Merged

n-dering mentioned this pull request Jul 19, 2025

Bump on-headers and compression n-dering/snake_react#2

Open

bjohansebas mentioned this pull request Jan 26, 2026

feat: enable compression middleware for http/2 webpack/webpack-dev-server#5638

Merged

Bugfix/use write head instead of implicit header by Icehunter · Pull Request #170 · expressjs/compression · GitHub

Uh oh!

Uh oh!

Bugfix/use write head instead of implicit header - #170

Bugfix/use write head instead of implicit header#170bjohansebas merged 19 commits intoexpressjs:masterexpressjs/compression:masterfrom Icehunter:bugfix/use_writeHead_instead_of_implicitHeaderIcehunter/compression:bugfix/use_writeHead_instead_of_implicitHeaderCopy head branch name to clipboard

Conversation

Icehunter commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougwilson commented Mar 2, 2020

Uh oh!

Icehunter commented Mar 2, 2020

Uh oh!

Icehunter commented Mar 2, 2020

Uh oh!

Icehunter commented Apr 13, 2020

Uh oh!

dougwilson commented Apr 13, 2020

Uh oh!

Icehunter commented Apr 16, 2020

Uh oh!

valoricDe commented May 5, 2021

Uh oh!

lamweili left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lamweili commented Jul 30, 2022

Uh oh!

lamweili commented Jul 30, 2022

Uh oh!

Icehunter commented Jul 30, 2022

Uh oh!

lamweili commented Aug 4, 2022

Uh oh!

lamweili commented Oct 7, 2022

Uh oh!

Uh oh!

lamweili left a comment

Choose a reason for hiding this comment

Uh oh!

bjohansebas commented Nov 9, 2024

Uh oh!

Icehunter commented Nov 9, 2024

Uh oh!

jonchurch commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonchurch commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Bugfix/use write head instead of implicit header#170
bjohansebas merged 19 commits into
expressjs:masterfrom
Icehunter:bugfix/use_writeHead_instead_of_implicitHeader

Icehunter commented Mar 2, 2020 •
edited
Loading

lamweili left a comment •
edited
Loading

jonchurch commented Nov 10, 2024 •
edited
Loading

jonchurch commented Nov 10, 2024 •
edited
Loading