Expose the socket in case of SocketError #1041
Conversation
delvedor
added
enhancement
|
I'm not sure how to add the socket info in |
|
I'm +/- 0. I would like to avoid exposing internals as much as possible so that in the future we could e.g. replace the socket implementation without breaking everything. Rather than exposing the socket directly could you create a new object that exposes the properties that you need? |
I don't see the problem, the
I fear this approach might not be future-proof. Today I need the local/remote address, tomorrow might be something else, which will require another pr to adds the new properties (and so on...). |
There was a problem hiding this comment.
Adding a reference to the socket might delay the time a socket is garbage collected. This is likely not a problem here as the fd has already been destroyed when this error is called, but it might happen in some other cases if we keep following this pattern elsewhere.
Ragarding @ronag concern about future support for different kind of Socket object... I don't think that would come with anything less than a semver-major change.
I'm ok in shipping this.
|
There is still #1041 (comment) that needs to be addressed. |
|
@delvedor you mean https://github.com/nodejs/undici/blob/main/lib/api/api-connect.js#L43? I think you'd need to pass the |
Codecov Report
@@ Coverage Diff @@
## main #1041 +/- ##
=======================================
Coverage 94.89% 94.89%
=======================================
Files 37 37
Lines 3623 3624 +1
=======================================
+ Hits 3438 3439 +1
Misses 185 185
Continue to review full report at Codecov.
|
|
@ronag any final objection |
* Expose the socket in case of SocketError * Updated test * Add missing socket * Address feedback * Make .socket nullable
* Expose the socket in case of SocketError * Updated test * Add missing socket * Address feedback * Make .socket nullable
If a
SocketErrorhappens, a user might want to dig deeper to understand why it has happened. For example, they might want to read the socket local/remote address of the socket.This pr adds a
.socketproperty to theSocketErrorwhich holds the socket that caused the error.