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

core: second in flight command fix #1862

Merged
merged 1 commit into from
Jul 18, 2022
Merged

core: second in flight command fix #1862

merged 1 commit into from
Jul 18, 2022

Conversation

julianoes
Copy link
Collaborator

In order to prevent too many commands from clogging the queue, we remove them if they look identical.

This changes it so that we only drop a command if it does not come with a callback. If it has a callback associated with it, it's better to call it anyway, otherwise odd races or just Denied results when you don't expect it tend to be the result.

This is the same as I fixed previously, however, this time also for CommandLong which I forgot last time.

In order to prevent too many commands from clogging the queue, we remove
them if they look identical.

This changes it so that we only drop a command if it does not come with
a callback. If it has a callback associated with it, it's better to call
it anyway, otherwise odd races or just Denied results when you don't
expect it tend to be the result.

This is the same as I fixed previously, however, this time also for
CommandLong which I forgot last time.
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

But then isn't there the problem that when we run something like REQUEST_MESSAGE every second, but it takes 3 seconds to time out, then we keep growing the queue?

@julianoes
Copy link
Collaborator Author

then we keep growing the queue?

Yes, but for requests that we do every few seconds, we usually do it without callback. And if we do it with callback then we would realize that it doesn't work or takes a long time, and could fix it.

@julianoes
Copy link
Collaborator Author

I'm merging this so that we at least match CommandLong and CommandInt.

@julianoes julianoes merged commit 6ec819d into main Jul 18, 2022
@julianoes julianoes deleted the pr-command-long-fix branch July 18, 2022 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants