-
-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix usage of undocumented _implicitHeader #128
Fix usage of undocumented _implicitHeader #128
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Yea, you can go ahead and update the Travis CI file. We just have one entry for the latest minor for each major. I'm not sure if any of the 8.x ones have the http2 module, but at least adding the most recent 9 would work. Make sure to validate all functionality works with 9.x, including no deprecation message are printed, for this and all our dependencies before adding it to Travis CI 👍 that is our standard Travis CI process. |
Thanks for checking, @maritz . Can you verify that you ran every test suite of even dependency of this module throughout the tree with Node.js 9.x as well to check this? This module doesn't fully exersize all the features of our own dependencies because the assumption is that they already have test suites, so we don't want to copy-and-paste every test suite each module up. Many times the issue is only apparently when doing this, so we have to do this in order to bump any version in the Travis CI configuration 👍 |
Do you mean going through all 12 dependencies, check out their git repo and manually run their tests? If so, no. I don't have the time to do that right now and quite frankly don't think that's a reasonable thing to do. Or do you have some tool to do this? Or did I misunderstand? |
Yes, that is the correct understanding. It's no problem at all, I can do it 👍 I just didn't want to if you already did it. I just use some shell scripts that use |
Ok, I ran the tests of everything against Node.js 9.4 and updated the Travis CI in |
Hm, not sure why the tests are suddenly failing on Travis. Works fine locally with 9.4 and worked fine on Travis before merging the master to resolve the conflicts. |
Yea, I agree looks like nothing changed. Perhaps the test itself just has a race condition and it just happened to fail this time? If rerunning the test it suddenly passes, probably a flaky test that would need to be debugged. |
Okay, now the tests passed. I'm hoping that these changes actually fixed it. Can't guarantee it though, since I don't truly understand what caused it in the first place. Anything else I could do? |
@dougwilson Could you manually trigger the travis checks for this PR a couple of times to see if it still fails occasionally? Not a great thing to have to do, but better be safe, right? |
Yep, I can do that 👍 sorry for the delay, we're just trying to get out a couple security patches out asap. Jan seems to be the season of receiving security reports or something 😂 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There were changes in node v9.8.0 that have the potential to maybe fix this... That's about all I got for this right now. :-( |
I'm actually occasionally seeing this in the one app where I use this PR. I have no indication yet what causes it and it always takes the entire process down. If anyone has any ideas on how to debug this, I'm very interested in it. |
d7bb81b
to
cd957aa
Compare
e4331cc
to
3b4a73e
Compare
@maritz, @dougwilson: need any help with this PR? I'd definitely prefer using this over rolling our own compression support for HTTP2. |
If you can figure out what is causing the seemingly random failures, that would be great. I don't have the time and honestly probably lack understanding of how compression and http2 work to fix this properly. |
@maritz: do you have any hints on what your app does that might be provoking those failures? So far, I've been unable to reproduce that failure. It may be that the version of node 10 is different than what you were using. I see that some fixes (such as nodejs/node#23146) have been made to HTTP2 since you saw this happening. |
Not really, it was just failing after X amount of time working fine. It's a next.js server that also serves a few static files. So nothing too fancy. But the Travis CI also had failures sometimes. So I really don't know how to approach this. |
I'll be doing a bunch of testing of your published fork within our app soon; maybe that will be sufficient validation. Will report back. |
With current commit, I could not reproduce the error @maritz reported.
Fixing the second issue, I think there are two approaches. Any thoughts? |
do you have any ETA to get this merged? |
I have re-enabled compression on my server again with a current node version (v12.14.1) and have performed some long-running load tests on a locally running next.js server serving html (200 status code) with >1m of compressed requests. I have also done a lot of manual loading of 200 and 304 requests with gzip compression on. So far no crashes. I don't know if something changed in node or whether I've just been unlucky in reproducing it this time. I vote for merging and then dealing with potential issues if they arise. Since all failures I've personally seen were with http2 anyways - not http1 - this would still be an improvement over the status quo of no one really using compression over http2 in the express-based world. Otherwise, if anyone is interested in testing it without an official merge, you can use '@maritz/compression' from npm and report back how it works out for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for requesting my review. At the time, I was hoping that @sogaani 's comments would be answered prior to merging and then... promptly forgot to follow up here 😆 I'm taking from your response that there is nothing to do from his comments, so just reviewed the tests that were added and also wanted to note I do have a "rebase" label on this issue, as it is currently merge conflicts preventing merge. If you have any follow up questions / comments I will be back around tomorrow to follow up 👍
var _compression = compression(opts) | ||
var server = http2.createServer(function (req, res) { | ||
req.on('error', function (error) { | ||
console.error('An error event occurred on a http2 request.', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.
console.error('An error event occurred on a http2 request.', error) | ||
}) | ||
res.on('error', function (error) { | ||
console.error('An error event occurred on a http2 response.', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.
}) | ||
}) | ||
server.on('error', function (error) { | ||
console.error('An error event occurred on the http2 server.', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.
console.error('An error event occurred on the http2 server.', error) | ||
}) | ||
server.on('sessionError', function (error) { | ||
console.error('A sessionError event occurred on the http2 server.', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one is going to comb through the CI logs after every run or upgrade... do these need to be here? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.
function createHttp2Client (port) { | ||
var client = http2.connect('http://127.0.0.1:' + port) | ||
client.on('error', function (error) { | ||
console.error('An error event occurred in the http2 client stream.', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.
// 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.setTimeout(1, function () { | ||
console.warn('An http2 connection timed out instead of being closed properly.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the comment says "force existing connections to time out after 1ms.", but inside the timeout there is nothing but a console warn. Should there be something being don in here? Otherwise, what purpose does the timeout serve? If to fail the test, should an error be passed to callback instead to make the test fail?
assert.equal(headers['content-encoding'], 'gzip') | ||
}) | ||
request.on('error', function (error) { | ||
console.error('An error event occurred on a http2 client request.', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.
console.error('An error event occurred on a http2 client request.', error) | ||
}) | ||
request.on('data', function (chunk) { | ||
// no-op without which the request will stay open and cause a test timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this assert that the data was actually correct?
Yeah, I'll try to see what I can do about that. I obviously can't really make a decision regarding whether to check for 304 in the module or in node compat. But I can do some more tests with old node versions etc. to see if the 100% reproducible cases can be reasonably added to theses tests.
My bad, I saw that at some point and then forgot about it later on. :-D Regarding the test reviews: Yep, those logs and no-ops are suboptimal. I think I added the logs simply for the case that we do see the unexpected failures and they could be caught with this. But it's been a long time, I'll go over all the tests and make sure they are done properly. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
If someone else wants to take over, of course they are free to do so. I can't promise any time frame about when I'll get around to this, sorry. |
@maritz You can check a box I believe bottom right of the PR to allow maintainers to push to this PR, if youd like to leave the work open to updates. |
@jonchurch Thanks, already checked by default, it seems. 👍 |
@maritz @jonchurch I can't rebase this PR however master...Icehunter:bugfix/use_writeHead_instead_of_implicitHeader I did fork @maritz 's fork; and then rebased from express master. How can I help with this? |
@Icehunter You can either submit a PR to OP's branch https://github.com/maritz/compression/tree/bugfix/use_writeHead_instead_of_implicitHeader or create a new PR here |
Hey, I am running into this issue as well. It seems @Icehunter has this all already addressed apart from one edge case? Is there a way to help here and make this merge happen, as we are otherwise at a loss if we don't use express. 😢 |
Superseded by #170. |
Close in favor of #170 |
As discussed in #127 this PR now only includes the changes that should not break node v0.8 and has tests.
Sadly supertest does not support http2 yet, so I had to resort to vanilla server/client creation.
I'm not destroying the client/server here, not sure if that's needed?!