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 stop_on_error_timeout blocking other messages in queue #539

Merged
merged 3 commits into from
Aug 24, 2020

Conversation

glentakahashi
Copy link
Contributor

@glentakahashi glentakahashi commented Aug 18, 2020

In the current implementation, stop_on_error_timeout doesn't work as expected and instead completely blocks the message queue rather than operating as an async callback as intended.

Explanation:
stop_on_error_timeout = 0.1
If we send 100 requests at 1 per 0.01 second, and an error occurs after 0.3 seconds (request 30), this is what is expected to happen:

  • requests 1-30 should be queued as normal
  • requests 31-40 should be aborted
  • requests 41-100 will be queued as normal

What actually happens:

  • requests 1-30 are queued as normal
  • the message queue blocks for 0.1 seconds
  • requests 31-100 are queued as normal

A more extreme case can be seen if you bump this up to stop_on_error_timeout = 15. What happens in this case is:

  • requests 1-30 are queued as normal
  • the message queue blocks for 15 seconds! - No communication happens at this time, from the outside it appears as if the kernel is stalled.
  • requests 31-100 are queued as normal

this PR fixes this by moving the "finish abort" callback to the event loop instead of using the existing message queue

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a reasonable change to me, but I'll leave it open for a bit in case anyone else has any concerns.

@blink1073 blink1073 added this to the 5.5 milestone Aug 24, 2020
@blink1073
Copy link
Contributor

Thanks again!

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