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

Make pending activations interrupt long poll #89

Conversation

Sushisource
Copy link
Member

@Sushisource Sushisource commented Apr 11, 2021

What was changed:

Pending activations need to take priority over any currently in-progress but not yet completed poll. This fixes that.

Why?

Cancels of activities were getting lost because of the long poll

Checklist

  1. Closes issue:

  2. How was this tested:

  1. Any docs updates needed?

_ = shutdownfut => {
Err(ShutdownErr.into())
}
_ = pa_fut => {
Copy link
Member

Choose a reason for hiding this comment

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

What happens to the poll request in this case?
Is it cancelled? Is it ignored?
We probably don't want to create a new request if we got interrputed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a good question, was thinking about it after I wrote this. I'll see if I can figure out a way to test that while I finish this. Ideally, it doesn't really matter, because we spit out the PA so fast that when we go back to requesting it'll kinda be like nothing ever happened... but perhaps we need to keep the polling future around in order to ensure that behavior. I'll play with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this opens up too big of a can of worms to deal with in this PR. Ideally the request should actually be cancelled I think. Keeping it around raises too many questions like "what if the next poll is for a different task queue" or "how long should we buffer the response" and so on. I opened #91 so we can come back to this

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should discard initial poll as from the lang's perspective it polled and got an activation, it shouldn't care if core made a request to the server or not in order to get it.

@Sushisource Sushisource marked this pull request as ready for review April 12, 2021 21:09
@Sushisource Sushisource changed the title Make pending activations interrupt long poll (daft, needs polish) Make pending activations interrupt long poll Apr 12, 2021
@vitarb vitarb merged commit 037a62f into temporalio:master Apr 13, 2021
@Sushisource Sushisource deleted the fix-pending-activtations-not-interrupt-poll branch April 13, 2021 21:07
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.

None yet

3 participants