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

Use actions queue when dispatching undo actions from "Release Actions" #1853

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Oct 30, 2024

Fixes #1837.

@jgraham could you please check? I hope that this is fine - it at least matches what we use in Firefox now when dispatching the actions from the parent process and it works fine. Thanks.


Preview | Diff

@whimboo whimboo requested a review from jgraham October 30, 2024 09:11
@whimboo
Copy link
Contributor Author

whimboo commented Oct 30, 2024

Well, I actually missed that we also have to enqueue the retrieval of the undo actions.

@whimboo
Copy link
Contributor Author

whimboo commented Oct 30, 2024

This PR should be ready for review now. Thanks.

Comment on lines +10321 to +10337
<li><p>Let <var>token</var> be a new unique identifier.

<li><p>Enqueue <var>token</var> in <var>input state</var>&apos;s <a>actions
queue</a>.

<li><p>Wait for <var>token</var> to be the first item
in <var>input state</var>&apos;s <a>actions queue</a>.

<aside class=note>
<p>This ensures that only one set of actions can be run at a time,
and therefore different actions commands using the same underlying
state don&apos;t race. In a session that is only a HTTP session only one
command can run at a time, so this will never block. But other
session types can allow running multiple commands in parallel, in
which case this is necessary to ensure sequential access.
</aside>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgraham this is a bit of duplication but I hope it's fine. Otherwise we might have to create a new algorithm, that is just waiting for the token to be first in the queue.

Copy link
Member

Choose a reason for hiding this comment

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

I think factoring this out into a new algorithm would be better.

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.

"Release Actions" doesn't yet use the queuing mechanism for actions
2 participants