fix: connections and threads failing to close

[seanjb]

Hi there, great library, thanks for developing. I noticed that connections were staying open even after closing them. Preventing threads and connections from properly ending.

It would appear the connection close function is setting the ready state to closed after emitting the close event. https://github.com/Bugs5382/node-hl7-client/blob/51ae5bd58d98287189b875f83bfd05f2ea047175/src/client/connection.ts#L150-L152

This causes a failure on the listener trigger by the close event https://github.com/Bugs5382/node-hl7-client/blob/51ae5bd58d98287189b875f83bfd05f2ea047175/src/client/connection.ts#L334-L338

It will always run into this exception state as the readystate has already been set to closed.

Having tested locally a fix would be to remove the line https://github.com/Bugs5382/node-hl7-client/blob/51ae5bd58d98287189b875f83bfd05f2ea047175/src/client/connection.ts#L152

Even with this fix the process thread is kept open until connectiontimer timeout is next triggered. Root cause is here

Adding the following clearTimeout(this._connectionTimer); just before or after the below line would resolve this issue https://github.com/Bugs5382/node-hl7-client/blob/51ae5bd58d98287189b875f83bfd05f2ea047175/src/client/connection.ts#L150

With these two fixes the connections successfully close and the process thread terminates.

[Bugs5382]

@seanjb Do you want to submit a PR against the develop branch?

[Bugs5382]

 this._readyState = ReadyState.CLOSED 

It will have to stay in the metood of 'close()' method is called again, I don't see why it would, but just in case, it just stops the whole method from running. On the 'checkSend()' inside the 'sendMessage()' would sometimes fail if this is not set in close.

https://github.com/Bugs5382/node-hl7-client/blob/9d9f2965906417b4027640950e63d8d9ca7f5314/src/client/connection.ts#L150-L154

[Bugs5382]

:tada: This issue has been resolved in version 2.1.1-beta.3 :tada:

The release is available on:

• npm package (@develop dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[Bugs5382]

@seanjb - Please try the beta version above before I release it to the wild.

[seanjb]

Thanks @Bugs5382 this seems to have resolved the second part of the issue, however the first part still persists. The race condition.

Coming into the socket.on('close') method the connection ready state is already set to closed.

https://github.com/Bugs5382/node-hl7-client/blob/9d9f2965906417b4027640950e63d8d9ca7f5314/src/client/connection.ts#L336-L340

Having debugged the code I can see the below line is reached before the above method. https://github.com/Bugs5382/node-hl7-client/blob/9d9f2965906417b4027640950e63d8d9ca7f5314/src/client/connection.ts#L154

Causing a 'Socket closed unexpectedly by server' error. So perhaps a fix might be to add an extra check into the first if conditional e.g. (this._readyState === ReadyState.CLOSING || this._readyState === ReadyState.CLOSED) or this._readyState === ReadyState.CLOSING || !this._connectionTimer

Not sure of any other functionality this could break.

[Bugs5382]

@seanjb Looking into it.

[Bugs5382]

I think...

this._readyState === ReadyState.CLOSING || !this._connectionTimer

It will be the better choice. No connection timmer means it should have (in theory) already been closed manually—I plan on releasing a new build tomorrow. I have a test script that runs for about 24 hours with about a million hl7 messages for testing and simulated disconnects, etc., to make sure.

[Bugs5382]

@seanjb If you want to give it a shot, there's a new branch running with this code. As I said above, I have local testing going on right now. By the time I get up in the morning, I should have the results.

[Bugs5382]

106 PR

[Bugs5382]

:tada: This issue has been resolved in version 2.2.0-beta.1 :tada:

The release is available on:

• npm package (@develop dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[Bugs5382]

:tada: This issue has been resolved in version 2.3.0-beta.1 :tada:

The release is available on:

• npm package (@develop dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[Bugs5382]

@seanjb Will be released into the wild by the end of the day. My massive testing didn't find any issues.

[Bugs5382]

:tada: This issue has been resolved in version 2.3.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[seanjb]

Sorry @Bugs5382 based in Sydney so didnt get a chance to test the beta when it became available. Have just checked 2.3.0 now and still have the issue.

It looks like when clearing the timeout the this._connectionTimer is not set to null. So my suggested fix was insufficient (sorry about that). A better fix is as follows to check the connection timer if it exists has been destroyed - which is true if the cleartimeout method was invoked: (this._readyState === ReadyState.CLOSING || this._readyState === ReadyState.CLOSED) or this._readyState === ReadyState.CLOSING || (this._connectionTimer == null)) || (this._connectionTimer?.destroyed)) )

Tested the above through debugging and works.

It may also be safer to clear the timeout before emitting the close event also. However my testing did not reveal this to have any impact if it is before or after the emit.

[Bugs5382]

@seanjb Can you submit a 2nd PR?

[Bugs5382]

:tada: This issue has been resolved in version 2.3.1-beta.1 :tada:

The release is available on:

• npm package (@develop dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[Bugs5382]

@seanjb Added to develop. Use the developed version for now for testing. Tomorrow (25th my time) when I wake up, I'll check to see if everything is good. Thanks for your help, and be sure to ⭐ it up! 👍

[seanjb]

Cheers - just tested and Beta confirmed working

[Bugs5382]

Ok. I know I am up.. hah... but I have a few more things to merge into main tomorrow. So just going to hold off.

[Bugs5382]

:tada: This issue has been resolved in version 2.3.1 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: