-
Notifications
You must be signed in to change notification settings - Fork 467
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
Read acknowledgements send by Riemann #2523
Conversation
This user does not have permission to start the build. Can one of the admins verify this patch and start the build? |
1 similar comment
This user does not have permission to start the build. Can one of the admins verify this patch and start the build? |
0a6076d
to
40460bd
Compare
@kira-syslogng ok to test |
Build FAILURE |
Error 404 when I try to see the failure 😱 |
@smortex thanks for the investigation and for the pull request. I just tested and unfortunately it crashes for me in two cases. I am not sure about the root cause but these may be connected. I got a clue with double free variant executed with valgrind:
so maybe there is some trouble about the ownership of
With a simple configuration:
Crash condition:
With a little more complex configuration: send messages in batches of 5, with 5s timeout, if the batch is not full:
There is a variant when syslog-ng is stoppend after the timeout, I had a crash in that case too, but got the same backtrace as when the batch size was 1. |
@smortex yes, you cannot check the result because of permission settings |
40460bd
to
99026da
Compare
Build SUCCESS |
99026da
to
c3ef042
Compare
Edit: Problem fixed
@furiel I added a commit that seems to improve the situation (after a rebase on top of master and --signoff of my previous commit).
Yet, I seem to have a crash that seems to be beyond the scope of this PR when I:
syslog-ng seems to crash after it reconnects. My configuration:
while echo 'moin'; do sleep 0.2; done | sudo ./syslog-ng/syslog-ng -F --module-path ./modules/affile/.libs/:./modules/riemann/.libs:./modules/syslogformat/.libs
So the condition seems to be similar to the one the workaround fix, the message holds an event which address is not valid. |
Build SUCCESS |
c3ef042
to
98838d2
Compare
@furiel Nailed it! Can you please test this and confirm it's working as expected? |
Build SUCCESS |
@smortex it works indeed. Please squash the second commit into the first so that the code remain bisect-able. |
When Riemann receive a message, it sends an acknowledgements to the sending process when the connexion is over TCP (TCP or TLS mode of operation). Not reading this acknowledgement make these messages fill-in the Recv-Q on the system running syslog-ng. The riemann_communicate() function of riemann-c-client is a wrapper around riemann_client_send_message_oneshot() that also reads this acknowledgement if it exist, or mocks it when using UDP. This patch change the riemann module to rely on riemann_communicate() to send events and avoid filling-in the Recv-Q. Because the function returns a more granular riemann_message_t, update the trace message to include these details, and adjust the error detection code accordingly. Fixes syslog-ng#2521 Signed-off-by: Romain Tartière <romain@blogreen.org>
98838d2
to
c4ce16c
Compare
🎉 Here we go! |
Build SUCCESS |
When Riemann receive a message, it sends an acknowledgements to the sending process when the connexion is over TCP (TCP or TLS mode of operation). Not reading this acknowledgement make these messages fill-in the Recv-Q on the system running syslog-ng.
The
riemann_communicate()
function of riemann-c-client is a wrapper aroundriemann_client_send_message_oneshot()
that also reads this acknowledgement if it exist, or mocks it when using UDP.This patch change the riemann module to rely on
riemann_communicate()
to send events and avoid filling-in the Recv-Q. Because the function returns a more granularriemann_message_t
, update the trace message to include these details, and adjust the error detection code accordingly.Fixes #2521