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

Add a wait until all data is sent, before closing socket #81

Merged
merged 1 commit into from
Aug 13, 2023

Conversation

nazarkhanov
Copy link
Contributor

Problem: not all logs reach logstash, when using tcp connection. moreover, all the logs reach the locally working logstash, and when connecting to a remote one, only part of the logs come (in our case)

Cause: the self._sock.close() function call closes the socket before all data from the socket write buffer has time to go over the network.

Adding this fix helped us avoid loss of logs

@eht16 eht16 merged commit 98b11d5 into eht16:master Aug 13, 2023
4 checks passed
@eht16
Copy link
Owner

eht16 commented Aug 13, 2023

Thanks, I could reproduce the described behavior and also that the changes fixed the loss of the send buffer.

I wonder why I never noticed this and I actually never expected this.

While testing, I noticed also reproducable errors on the Logstash side(with and without your changes):

[2023-08-13T16:31:35,401][ERROR][logstash.inputs.tcp      ] somehost/8.8.8.8:52642: closing due:
java.net.SocketException: Connection reset

Those can be fixed when we tell Logstash that we are going to close the socket.
I just pushed a further change to call socket.shutdown() which fixed the 'Connection reset' for me.

Would you mind testing this change?

@nazarkhanov
Copy link
Contributor Author

I couldn't immediately reproduce the error "java.net.SocketException: Connection reset", but by switching to the logstash version on 8.4.1, I was able to reproduce it. On logstash version 7.16.1, only the info message "closing (Connection reset by peer)" was shown. Calling socket.shutdown() really fixed this error on these versions.

Could you release these changes?

@eht16
Copy link
Owner

eht16 commented Aug 20, 2023

Thanks for the feedback.

Just released the changes in 2.7.0.

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

Successfully merging this pull request may close these issues.

2 participants