Handle stream error in StandaloneConnector.js #1236
Conversation
|
@peteraw77 thanks for the PR, but it does definitely need a test case. Error handlers are typically assigned later on in the chain once the connection was created. I dont remember the reasoning for not having an error handler in the create helper, either way a test covering this would be required to get this merged |
|
@AVVS where would be the right place to put the test case? |
|
Added a test in test/functional/connection.ts but I'm having a hard time getting the tests to reproduce the error. This was my best attempt but it's definitely not ideal. |
test/functional/connection.ts
Outdated
| const stub = sinon | ||
| .stub(net.Socket.prototype, "connect") | ||
| .callsFake((options) => { | ||
| net.Socket.prototype.emit("error"); |
There was a problem hiding this comment.
this needs to be async, .connect() doesnt throw in the same tick - wrap this in process.nextTick
There was a problem hiding this comment.
Thanks, it seems to be capturing the behaviour I intended it to now
|
Is there an expected timeline on when this will be merged? This issue has been causing endless headaches for our team |
|
Ping here. I would be great to merge this fix soon. It generates Unhandled errors, which are hard to debug. In our case it was an error: It takes us time to find that bug comes from this issue. |
Same here, it's such a simple, obvious but crucial fix... |
|
Please merge that change as we had to write dedicated service to catch those unhandled errors... |
|
Also pinging! If I was a maintainer I would merge. But alas I am a simple consumer of this great package. |
|
Sorry for the late response. I didn't notice this PR... Will take a look soon! |
Currently, a stream (ex `net.Socket`) is returned via a Promise, and error handlers are attached to the stream in the Promise callback. However, if the errors are thrown immediately (ex with `process.nextTick()`), the error handlers will not have the chance to attach to the stream, thus unhandled error events will be thrown. This PR makes all connectors resolve a factory instead of the created stream, so the event handlers can be attached in the same tick. Closes #1236 Closes #1209 Closes #1289
|
I created a PR to address the root issue: #1296. I'll leave this open and may merge this if I can't get #1296 done very soon. Thank you for the contributions! |
|
🎉 This issue has been resolved in version 4.24.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
You the man @luin ! |
Solution proposed by @OPScorp in #1209 . Solves #1235 as well. I'm not sure why he didn't PR this himself but it was helpful for myself and others so I would like to get it merged.