Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCP connection termination #51

Closed
Fritzelblitz opened this issue Jul 7, 2022 · 7 comments
Closed

TCP connection termination #51

Fritzelblitz opened this issue Jul 7, 2022 · 7 comments

Comments

@Fritzelblitz
Copy link

If a TCP connection opened via MLLP.Client.start_link is closed by the remote side before we could send the message, the message will be sent to the closed connection and we will get the error message that the connection was closed. After that a connection is re-established and after some time it is closed again by the remote side. As far as I understand the connection behavior of the MLLP.client, this behavior is expected.

Would it make sense that MLLP.Client.send always tries to re-establish the connection when trying to send if the connection is not established?

@Fritzelblitz
Copy link
Author

I tried an explicit call to MLLP.Client.reconnect, but this did not work as expected #52

@starbelly
Copy link
Contributor

@Fritzelblitz do you think you still need the behaviour where by when you send and it's not connected it should attempt to connect?

@vikas15bhardwaj what do you think about this?

@Fritzelblitz
Copy link
Author

With the patched reconnect function, I would no longer need the feature.

However, I still think it would be useful, it doesn't seem to make sense to send messages to a closed connection. At least as an option for the MLLP GenServer to reopen the connection on a send when it is closed would make sense.

@vikas15bhardwaj
Copy link
Contributor

@starbelly I think it would be OK for send to reconnect if it sees client is not connected anymore. This will avoid apps worrying about reconnection. Do you see any downside with it?

@starbelly
Copy link
Contributor

@vikas15bhardwaj Nah, I don't think so, other than implicit behaviour. I think we should support this.

@starbelly
Copy link
Contributor

I think this might be resolved via #68 , @Fritzelblitz maybe you can try that out before it gets merged?

@starbelly
Copy link
Contributor

I'm pretty sure the above mentioned PR which has been merged solves this issue. We will release this after a fair bit of testing. However, going to close this issue, if you feel this doesn't solve the problem, please re-open it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants