Skip to content

Fix OperationQueue scheduling logic to prevent disruption of operation lists #2930

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

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

lxbndr
Copy link
Contributor

@lxbndr lxbndr commented Nov 27, 2020

OperationQueue would crash or hang if we force "non-natural" execution order. E.g. when first operation depends on second.

The reason is how we work with internal operation lists in OperationQueue._schedule(). I believe the original intention was to adjust __nextPriorityOperation, but instead we are modifying __nextOperation. This has no sense and leads to crosslinking of the main operation list and corresponding priority operation list. Subsequent scheduling would try to read already finished operation and hang. Also, at this point queue doesn't hold operation object anymore, so trying to read dead object is pretty realistic scenario.

Suggested test case checks scenario described above, and ensures correct operation order as a bonus.

…n lists

I believe the original intention was to adust `__nextPriorityOperation`.
Doing so for `__nextOperation` has no sense and leads to crosslinking
of the main operation list and corresponding priority operation list.
OperationQueue in such state would crash or hang while attempting
to schedule subsequent operation.
@lxbndr
Copy link
Contributor Author

lxbndr commented Nov 27, 2020

cc @compnerd @millenomi @spevans

@spevans
Copy link
Contributor

spevans commented Nov 27, 2020

@swift-ci test

@lxbndr
Copy link
Contributor Author

lxbndr commented Nov 30, 2020

Build errors look unrelated. @spevans could you please trigger new CI run?

@spevans
Copy link
Contributor

spevans commented Nov 30, 2020

@swift-ci test linux

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.

3 participants