Improve tests for Node.js clients and fix hanging requests #619
Conversation
timostamm
force-pushed
the
tstamm/fix-hanging-node-requests
branch
from
e183a09 to
aa83250
Compare
| /** | ||
| * Returns a ConnectError for a HTTP/2 error code. | ||
| */ | ||
| export function connectErrorFromH2ResetCode( |
There was a problem hiding this comment.
Unrelated to the bug fixes.
This is a continuation of #552, where we closed a cancelled request with the HTTP/2 code CANCEL. Now we also interpret HTTP/2 RST codes received by the client correctly.
| sentinel.catch((reason) => | ||
| response.destroy(connectErrorFromNodeReason(reason)) |
There was a problem hiding this comment.
In #552, we started handling abort signals in the client ourselves. We do the same here now, to reliably close the request on cancellation.
Without this change, the tests added in this PR fail.
There was a problem hiding this comment.
Will this address the issue of explicit cancellations from the client and how the server handles those?
| stream.on("close", function h2StreamClose() { | ||
| const err = connectErrorFromH2ResetCode(stream.rstCode); | ||
| if (err) { | ||
| sentinel.reject(err); | ||
| } |
There was a problem hiding this comment.
This makes sure we always raise an error if the request dies.
Without this change, the tests added in this PR fail.
| try { | ||
| await client({ | ||
| url: "http://example.com", | ||
| describe("universal node http client", function () { |
There was a problem hiding this comment.
These added tests spin up a Node.js HTTP/1.1 or HTTP/2.2 server that aborts incoming requests. We run a universal Node.js client (which is used by all Transports from @bufbuild/connect-node) against the servers. We test closing the request at several stages, for example before the first response byte is written, or while the request body is still read.
| reason instanceof ConnectError && reason.code == Code.Canceled | ||
| ? H2Code.CANCEL | ||
| : H2Code.INTERNAL_ERROR; |
There was a problem hiding this comment.
No logical changes here, just making use of the added enumeration for error codes.
| signal?.reason ?? | ||
| new ConnectError("This operation was aborted", Code.Canceled) |
There was a problem hiding this comment.
Unrelated to the bug fixes. With Node.js v16 being EOL soon, we can make better use of AbortSignal.reason.
| sentinel.catch((reason) => | ||
| response.destroy(connectErrorFromNodeReason(reason)) |
There was a problem hiding this comment.
Will this address the issue of explicit cancellations from the client and how the server handles those?
Do you mean #576? No, it does not. |
Ah no, I meant this discussion: #315 (comment) (I think they're different issues). |
Investigating #463 and #576, I found situations where both HTTP/1.1 and HTTP/2 client requests on Node.js can hang. This PR fixes them.
The issues were found by adding test coverage. We do not have definitive proof that this fixes #463 because we do not have a reproducible case, but I am fairly confident that it does.