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

fix: remove deadlock possibility by adding resend_queue queue #73

Merged
merged 5 commits into from
Mar 9, 2022

Conversation

sumo-drosiek
Copy link
Contributor

@sumo-drosiek sumo-drosiek commented Mar 8, 2022

In order to send data simultaneously, one of two approaches can be used here:

  • rewriting code to be explicitly multithred
  • Use async as it is done already

This PR keeps the async approach and prevent deadlock by adding additional queue (sized twice as worker numbers)

I wanted to add minimal set of changes in order to fix #71

Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek marked this pull request as ready for review March 8, 2022 14:31
@sumo-drosiek sumo-drosiek requested a review from a team as a code owner March 8, 2022 14:31
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek force-pushed the drosiek-fix-deadlock branch from e3acc5f to 38f0f49 Compare March 8, 2022 14:33
Copy link
Contributor

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to see some tests for it :)

Dominik Rosiek added 2 commits March 9, 2022 11:45
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@swiatekm
Copy link

swiatekm commented Mar 9, 2022

Instead of doing this, couldn't we just make the requeue non-blocking and drop data if there's no space left in the queue? Do we know what the right thing to do in general is for a Logstash output if it can't send data fast enough? Should it block, return an error, or something else?

@sumo-drosiek
Copy link
Contributor Author

AFAIK if queue is full it is going to backpressure logstash, so no new events are going to be put into queue until it has space

Should it block, return an error, or something else?
not sure, so leaving main architecture as it is

I want to avoid changing architecture or behavior if there is no need to do that

@sumo-drosiek
Copy link
Contributor Author

I'm merging this PR as we need bugfix urgently. We can continue discussion here and add changes as separate PRs

@sumo-drosiek sumo-drosiek merged commit 76cbbc0 into main Mar 9, 2022
@sumo-drosiek sumo-drosiek deleted the drosiek-fix-deadlock branch March 9, 2022 12:04
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.

Agent does not send any data after throttling event
3 participants