fix tcp socket stream destruction #126
Conversation
|
Initial build failed due to outdated CodeBuild environment image. |
|
|
||
| public async init(): Promise<void> { | ||
| return new Promise<void>((resolve, reject) => { | ||
| this.socket.connect(this.endpoint.port, this.endpoint.host, (err?: Error) => { |
There was a problem hiding this comment.
So, if I understand this correctly, by doing an early connect attempt here we cover the case where the socket was already connected. We just throw so that the next connect can work? How exactly will this guarantee that close is emitted before we try to connect next?
There was a problem hiding this comment.
We need to get rid of the writable: false socket property which was causing the socket to be in readOnly mode for newer node versions, but doing that brings back this issue. To avoid that we need to connect to the socket after creation as recommended by the node docs.
There are 5 states the socket can be in: open, opening, readOnly, writeOnly and closed.
- The
openandopeningstates are already being handled by the code. - The
readOnlyandwriteOnlystates won't happen since we aren't setting the socket toreadableorwritableanywhere. - The
closedstate callsconnect()or on error callsdisconnect()and then tries to reconnect.
src/sinks/connections/TcpClient.ts
Outdated
| .setEncoding('utf8') | ||
| .setKeepAlive(true) | ||
| .setTimeout(5000) // idle timeout | ||
| .on('timeout', () => this.disconnect('idle timeout')) | ||
| .on('end', () => this.disconnect('end')) | ||
| .on('data', data => LOG('TcpClient received data.', data)); | ||
| this.init.apply(this); |
There was a problem hiding this comment.
Having init() & warmup() adds unclarity on what these methods are doing.
It seems a bit surprising that the socket would start making calls in the constructor, before the object is even fully constructed and returned.
Would making these changes inside warmup() be enough to catch this edge case?
There was a problem hiding this comment.
I agree that it might be confusing to have those two functions there, but putting the initial connection in warmup() won't be possible since getSocketClient() which is calling warmup() is a synchronous function that cannot be changed to an asynchronous one (called in AgentSink constructor).
I could rename init() to firstConnect(), initialConnect(), createConnection() or something similar for more clarity.
There was a problem hiding this comment.
As discussed, doing in the constructor is not optimal but it's an stablished pattern in this repo. So not a big issue.
firstConnect() or initialConnect() is better, as long as there's a comment explaining why it's needed.
Issue #, if available: #120
Description of changes:
Changes made to how the socket initially connects to its endpoint.
As per Nodejs documentation, "[socket.connect()] should only be used for reconnecting a socket after 'close' has been emitted or otherwise it may lead to undefined behavior".
The previous version was at times calling
socket.connect()without thecloseevent being emitted. This would sometimes try to connect to a socket that is already connected (EISCONN) and would destroy the stream. Additionally, thewritableproperty innet.Socket()was removed since it was causing socket streams on node >=v15.3.0 to start asreadOnlyand with an additionalconnect()attempt would lead to undefined behaviour.To avoid the same issues pre 0034ce1, an async
init()method within the constructor was added to initialize the connection once a TCP Socket is created.Additionally, fixed critical, high and moderate severity dependency related security alerts: #124, CVE-2021-43138, CVE-2021-3807, CVE-2021-3918, CVE-2021-44906, CVE-2021-3777, CVE-2021-23343, CVE-2020-28469
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.