-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: add error when destroying stream to generate close event even when the network connection is down #713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...e network connection is down
Codecov Report
@@ Coverage Diff @@ ## master #713 +/- ## ====================================== Coverage 92.9% 92.9% ====================================== Files 8 8 Lines 705 705 Branches 171 171 ====================================== Hits 655 655 Misses 50 50
Continue to review full report at Codecov.
|
A unit test would be really good at this point. You can at least add a test that checks that an 'error' event is emitted on the stream.
Is there any update to this one?
@mcollina Sorry, slammed by work - I hope I can get back to that and add the test later, although I'm not sure I understand yet what exactly you think it'll test (I have to spend more time looking at it)
feel free to close if you don't like idling issues.
More or less I want a test that verified that 'error' is emitted on the stream if this.end(true) is passed in, and such callback is called.
Also, an example showing that such callback is indeed not called without this change.
@pierreca any chance on the conflicts being resolved? If not can this please be closed and the associated issue updated so someone else can take a stab at this.
@pierreca Could you have another look at this please? I'm tring to keep this library updated and I'm looking at open PR to see if there are some that could be merged. This looks ok to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test that reproduces the issue
It's not 100% clear to me why this happens. I mean based on nodejs docs the close event should be emitted every time destroy is called on a stream, it's just the error event that is emitted too if an error is passed. I keep this opened for now as I got some errors too in the past with client.end and I'm not sure if this could be a solution to that or nope
also please move the PR to main instead of master
Uh oh!
There was an error while loading. Please reload this page.
Fixes #710
When a stream is closed because the network connection, the close even fires correctly but after that if a user calls
client.end()with a callback. that callback will never get called. As far as I understand, that is because the stream object will only fire another close event (which is required for the callback to be called) ifstream.destroy(). is called with an error argument.Tried to find a way to simulate the conditions in unit tests but could only repro the behavior with an actual network disconnection... so no specific test. existing tests don't seem to bother though.