Do not abort a an already aborted request, do not emit an error twice #886
Conversation
|
I have test-failures on my local machine, but they also occur without this commit. Let's see, if the tests pass in travis |
|
I have to check, why my new code is not covered by the test. It should be. |
nknapp
force-pushed
the
master
branch
2 times, most recently
from
1dd1aa2
to
ba3fbc6
Compare
|
I think, it is ready now... |
There was a problem hiding this comment.
Thanks for taking the time to do this @nknapp! Please respond to comments when possible and maybe we can get this shipped this week.
tests/test_abort.js
Outdated
| @@ -57,3 +57,28 @@ test("abort is emitted before delay time", function(t) { | |||
| req.abort(); | |||
| }, 10); | |||
| }); | |||
|
|
|||
| test("nock#882: Aborting an aborted request should not emit an error", function(t) { | |||
There was a problem hiding this comment.
No need to include the issue number
| setTimeout(function() { | ||
| t.equal(errorCount, 1,"Only one error should be sent."); | ||
| t.end(); | ||
| },10); |
There was a problem hiding this comment.
This seems brittle, since it relies on timing. Can we use req.on("end", () => { ... }) instead? That has the advantage of also asserting the request was completed.
There was a problem hiding this comment.
I tried that and got a test unfinished. It seems like the 'end'-event is not in this case. I also tried piping the response to a writable stream to make sure that the response is
Is an end-event supposed to be sent, when a request is aborted? But if "yes", then it would certainly be sent after the first abort, and not after the second one. In that case, the test could pass, even if the issue wasn't fixed, because it could happen that the t.equal-check is evaluated too early.
I think it is safer to use 'setTimeout` in this case.
|
I'll see to it, but probably not this week. I'll be travelling for the next couple of days and I don't know when I will find the time. |
Fixes nock#882 When a request is aborted, "nock" emits an "abort"-error. If the client reacts on that error by aborting the request (this is what "popsicle" does), the code will end in an endless loop. This commit makes that the the "abort"-code is only executed, if the request has not already been aborted.
|
I have removed the issue number from the test, and another unused statement. I don't think that the build errors in Travis are related to my changes. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you! |
Fixes #882
When a request is aborted, "nock" emits an "abort"-error.
If the client reacts on that error by aborting the request (this is what
"popsicle" does), the code will end in an endless loop.
This commit makes that the the "abort"-code is only executed, if the
request has not already been aborted.