search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.75k stars 28.3k forks source link

The differences between http2 compatibility API and http[s] #29829

Open sogaani opened 4 years ago

sogaani commented 4 years ago

Thanks to the efforts of making the compatibility API, differences between http[s] and http2 modules seldom annoy developers. But, some http2 compatibility API still behaves differently and lacks some APIs from http[s] and it causes breakages to ecosystems for http[s](e.g. https://github.com/expressjs/express/pull/3730).

In order to expose the differences, I've been translating test cases for http[s] to test cases for http2 at my repo. The following is a list of the differences currently founded by translated test cases.

I'm not sure all the differences have to be fixed. I thought if they should not be fixed, documenting the differences is good for the developer.

Differences between http and http2 compatibility API

ronag commented 4 years ago

I think you can add https://github.com/nodejs/node/pull/24347 to the list as well.

ronag commented 4 years ago

Really good work! I would be interested in helping sorting some of this.

ronag commented 4 years ago

@sogaani I would like to suggest we put your tests in under tests/known_issues so that we might sort them out later? @addaleax would you agree?

ronag commented 4 years ago

Here is another one that might be worth to add to the list, https://github.com/nodejs/node/issues/29529

sogaani commented 4 years ago

@ronag Thank you for looking at this issue. I added 2 issues you mentioned.

I'm glad to make pull request to put my tests at this repo if it is OK.

ronag commented 4 years ago

@addaleax Some guidance here? Would it make sense to add @sogaani's test in e.g. known_issues? Sorting all these out will probably take a while and I'd like to avoid losing this excellent work.

ronag commented 4 years ago

@sogaani: Another candidate for the list https://github.com/nodejs/node/issues/28267

ronag commented 4 years ago

@sogaani: Just wanted to give a quick ping that I'm still interested in helping sorting these out over time (a little swamped at the moment). But please do continue updating the list if you find more differences.

sogaani commented 4 years ago

@ronag Thank you for mentioning the difference. I added :)

ronag commented 4 years ago

@rexagod: If you are looking for tasks there is plenty to pick from here.

rexagod commented 4 years ago

Awesome! Thank you for pointing me here, @ronag!

rexagod commented 4 years ago

@addaleax @ronag I've marked sub-issues that exhibited intentional behavior as Expected behavior and submitted PRs for the rest of them. I think we can probably close this one now, or if any sub-issue was incorrectly marked, let me know and I'll separate that out into a new issue.

Fixes: https://github.com/nodejs/node/issues/29829

  1. https://github.com/nodejs/node/pull/33162
  2. Expected behavior
  3. Expected behavior
  4. Expected behavior
  5. https://github.com/nodejs/node/pull/33161
  6. Expected behavior
  7. https://github.com/nodejs/node/pull/33763
  8. https://github.com/nodejs/node/pull/33188
  9. https://github.com/nodejs/node/pull/33506
  10. https://github.com/nodejs/node/pull/33506
  11. https://github.com/nodejs/node/pull/33506
  12. https://github.com/nodejs/node/pull/33183
  13. https://github.com/nodejs/node/pull/33160
  14. https://github.com/nodejs/node/pull/31818
  15. https://github.com/nodejs/node/pull/30964
  16. https://github.com/nodejs/node/pull/33155
  17. https://github.com/nodejs/node/pull/33146
  18. Expected behavior
  19. Expected behavior
  20. Expected behavior
  21. https://github.com/nodejs/node/pull/33144
  22. Expected behavior
  23. https://github.com/nodejs/node/issues/33568
  24. Expected behavior
  25. Expected behavior
  26. [Working]
  27. https://github.com/nodejs/node/pull/29229
  28. [Working]
  29. https://github.com/nodejs/node/pull/24347
  30. https://github.com/nodejs/node/issues/29529
  31. https://github.com/nodejs/node/pull/32937
  32. [Bug already fixed and cannot be reproduced] (see https://github.com/expressjs/compression/pull/128#issuecomment-527730832). The following code snippet, close to sogaani's was used to confirm this.
    Code 
    const server = http2.createServer((req, res) => {
res.setHeader('Content-Type', 'text/plain')
res.writeHead(304)
res.write('a');
res.end()
});
server.listen(0, () => {
const client = http2.connect(`http://localhost:${server.address().port}`);
const request = client.request()
request.on('data', () => {})
request.on('end', () => {
    client.close();
    server.close();
})
})
clshortfuse commented 4 years ago

http2.Http2ServerRequest.connection.setTimeout Http2ServerRequest.setTimeout should return this instead of void/undefined

It's the last thing in the TypeScript typings to fix:

Argument of type 'Http2ServerRequest' is not assignable to parameter of type 'IncomingMessage'.
  The types returned by 'setTimeout(...)' are incompatible between these types.
    Type 'void' is not assignable to type 'IncomingMessage'.ts(2345)

See:

rexagod commented 4 years ago

Http2ServerRequest.connection.setTimeout returns a ServerHttp2Session/Stream. The code snippet below works.

req.connection.setTimeout(500).on('timeout', () => {
    console.log('timeout');
})

https://github.com/nodejs/node/blob/4678e44bb28c00dc22771a0ef2684a4d46715ab0/lib/internal/http2/compat.js#L206-L210

clshortfuse commented 4 years ago

Sorry, my mistake, I mean Http2ServerRequest.setTimeout, not Http2ServerRequest.connection.setTimeout

szmarczak commented 3 years ago

Also I think it would be nice to have http2stream.bytesWritten.

krokowski0x commented 3 years ago

@sogaani any updates on this? I wanted to help out with the http/2 in express proposal but this one seems to be a blocker

markb-trustifi commented 4 weeks ago

http2.Http2ServerResponse does not have res.writeProcessing

Isn't the 102 status code is totally deprecated and will not be added to HTTP/2 standard? https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/102#browser_compatibility Maybe it should be replaced with multiple res.write()?