Closed maxfliri closed 3 years ago
HTTP/2 in Got currently is very buggy as it uses outdated version of http2-wrapper
. I'll release a stable version today.
Ok, so everything is 100% finished but the docs. I'll do the docs later today and then release http2-wrapper@1.0.0
.
Released http2-wrapper@1.0.0
, you can apply this in Got via:
const http2wrapper = require('http2-wrapper');
const http2got = got.extend({
request: http2wrapper.auto
});
Also we don't use mocha here. I'm closing this for now. If the problem still exists, I'll reopen.
Thanks for looking into this. I just gave this a try: it fixed case 2, but case 3 is still failing.
Sorry about mocha, it's what we use here: the code example was adapted from a test out of our codebase. What would you prefer instead?
Here at Got we use ava: https://github.com/avajs/ava
Have you tried Node.js 15.10.0?
Ok, so I've managed to reproduce the issue. Working on this rn.
So I've investigated and figured out the following. The test name "throws an exception when connection to server is lost" is invalid. What actually happens is this: the stream is closed with rstCode 0
before the HEADERS frame is received.
What do you mean by invalid? That test tries to simulate what happens if the connection to the server is lost while a message exchange is in progress, without a graceful termination, such as, for example, in case of crash of the server, or in case of sudden loss of network connectivity.
That test tries to simulate what happens if the connection to the server is lost while a message exchange is in progress
Well, in reality the doesn't do that.
I understand that the test may not simulate that condition perfectly. I'm quite new to HTTP2 and I still have a lot to learn about it. But I think the test it's still a valid one that simulates a condition that can happen in the wild: a server can close the connection at any time, without following the protocol. The client library should detect the fact and handle it, for example by throwing an exception.
Ok so I released 1.0.3
which is 100% the same beta version. The new version is released as 2.0.0
. Sorry for the trouble. I had to do this because I didn't expect that the beta version specified in package.json would automatically bump up to 1.0.0.
I think the test it's still a valid one that simulates a condition that can happen in the wild
No, it's not valid because it doesn't do that. It just destroys a stream. And a stream is not a socket, the socket is still alive in this case. To test this properly, you'd have to destroy the entire server.
The client library should detect the fact and handle it, for example by throwing an exception.
It does. Check http2-wrapper@2.0.0
.
To test this properly, you'd have to destroy the entire server.
Like: ~server.destroy()
~. Call session.destroy()
on all sessions.
Brilliant! I upgraded to http2-wrapper 2.0.0, and now it works as expected.
a stream is not a socket, the socket is still alive in this case
Thanks for pointing this out, I see the mistake now. I misunderstood what stream.destroy()
does.
To test this properly, you'd have to destroy the entire server. [...] Call
session.destroy()
on all sessions.
I added this test to my suite and it works too.
Thank you so much for your help, I really appreciate the effort you put in and how quickly you sorted this out!
No problem :) Thank you for the third case, it was a tricky one!
Describe the bug
Actual behavior
When I send an http/2 request, got returns a response that remains in pending state forever, it never resolves nor rejects. This seems to happen when there is a problem with a network connection that was previously established successfully.
I have two failing test cases, reproduced below as a mocha test. The test sends some requests with
{ http2: true }
to a stub http server controlled by the test itself. The failing tests are the second and the third.Test case 2:
Interestingly this test passes when it's run on its own, but fails when I run all the tests. Given that the test runs after the first test, the behaviour is like this:
Test case 3:
This test always fails, even when run on its own.
Expected behavior
In both cases, the promise returned by got should be rejected with an appropriate error representing the underlying problem.
Code to reproduce
Checklist