fix: abort request when abort signal received #30
Conversation
In the case where responses don't end immediately, abort/destroy the request as well as the response as otherwise the connection is left open and the server never receives the 'abort' event for the request.
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
=======================================
Coverage 99.55% 99.56%
=======================================
Files 6 6
Lines 452 455 +3
Branches 147 148 +1
=======================================
+ Hits 450 453 +3
Misses 1 1
Partials 1 1
Continue to review full report at Codecov.
|
|
@arantes555 are there any changes you'd like to see here or can this be merged? |
|
Sorry for the delay, I completely missed this PR 🤦 I'll get on it! |
| if (request.useElectronNet) { | ||
| req.abort() | ||
| } else { | ||
| req.destroy() | ||
| } |
There was a problem hiding this comment.
| if (request.useElectronNet) { | |
| req.abort() | |
| } else { | |
| req.destroy() | |
| } | |
| if (request.useElectronNet) { | |
| req.abort() // in electron, `req.destroy()` does not send abort to server | |
| } else { | |
| req.destroy() // in node.js, `req.abort()` is deprecated | |
| } |
| if (!aborted) { | ||
| clearInterval(interval) | ||
| res.end() | ||
| throw new Error('request was not aborted') |
There was a problem hiding this comment.
I'm not a fan of this, as it causes an uncaught exception... In addition to not being good practice, when there is a problem, it causes the tests to fail in another test-case as the one that actually causes the problem.
|
@achingbrain I just pushed to another pull request : #31 I believe this PR addresses the problem you were trying to fix, while also resolving my concerns about your modifications. Can you confirm that what I'm proposing works for you? When confirmed, I will merge and publish |
|
It looks like it does, the test I added passes with the changes in #31 - thanks for picking this up. |
|
Closing in favour of #31 |
|
@achingbrain this has now been published as v1.7.2. (About the test you added, I removed it because I actually implemented the same logic of checking if the request actually appears to be finished for the server after every single test ^^ that's how I caught the timeout / encoding error thing) |
In the case where responses don't end immediately, abort/destroy the request as well as the response as otherwise the connection is left open and the server never receives the 'abort' event for the request.
I noticed this as I've got some http endpoints that send data over time rather than finishing up the request immediately, the solution seems to be to close the request stream on the client as well as the response body stream.