fix: add error when destroying stream to generate close event even when the network connection is down

[pierreca]

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) if stream.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.

[codecov]

Codecov Report

Merging #713 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #713   +/-   ##
======================================
  Coverage    92.9%   92.9%           
======================================
  Files           8       8           
  Lines         705     705           
  Branches      171     171           
======================================
  Hits          655     655           
  Misses         50      50

Impacted Files	Coverage Δ
lib/client.js	96.31% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d4d295...34873ca. Read the comment docs.

[mcollina]

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.

[mcollina]

Is there any update to this one?

[pierreca]

@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.

[mcollina]

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.

[OmgImAlexis]

@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.

[robertsLando]

@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

[robertsLando]

Please add a test that reproduces the issue

[robertsLando]

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

[robertsLando]

also please move the PR to main instead of master