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 command scheduling order consistent #5470

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Make command scheduling order consistent #5470

merged 2 commits into from
Oct 10, 2023

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Jul 25, 2023

Fixes #5467.
end is now called first along with interrupt actions on both languages. Sets are used to defer cancels, ensuring infinite loops aren't possible.

@Gold856 Gold856 requested a review from a team as a code owner July 25, 2023 20:40
@Starlight220
Copy link
Member

Starlight220 commented Jul 26, 2023

Also, the most important thing this PR should be adding is tests. Any possible edge case should be added and tested on both new and old impls; if there are differences they should be exactly what we want.

@Starlight220
Copy link
Member

I just realized that SchedulingRecursionTest.cancelFromInitialize doesn't check if end is actually called: it's an edge case that this PR might change, and if behavior has changed then we should be conscious of that.

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Glad to see more consistency! However, I have a few nitpicks (in addition to updating the PR description).

@Gold856
Copy link
Contributor Author

Gold856 commented Jul 30, 2023

Getting an error for the CancelDefaultCommandFromEnd test on C++:
Assertion failed: isHandleInSync() && "invalid iterator access!", file C:\Users\gold2\OneDrive\Documents\GitHub\allwpilib\wpiutil\src\main\native\thirdparty\llvm\include\wpi/SmallPtrSet.h, line 285

What's causing it?

@Starlight220
Copy link
Member

Probably concurrent modification.

@Gold856
Copy link
Contributor Author

Gold856 commented Jul 31, 2023

The CancelDefaultCommandFromEnd test on C++ actually found a problem: when cancelDefaultCommand schedules the other command, it will cancel the default command. The problem is that the cancel is deferred, but the requirements are moved anyways to the other command (which explains the failed assertion), and the other command is scheduled. After that, cancelDefaultCommand finishes its cancel, and the deferred cancel is also run.

Requirements are being transferred before the command actually ends; this seems to cause a problem for C++, and also might be problematic in general. However, the command is guaranteed to be canceled, so this might be fine to leave in. The test also passes in main, so there's behavior that needs to be preserved. How do we want to deal with this?

@KangarooKoala
Copy link
Contributor

Things have gotten kind of thorny, so I tried completely rethinking the whole thing, and here's the proposal I have: Instead of dealing with the queue headaches, use a set to keep track of which commands are currently ending to detect and prevent infinite recursion.

In more detail:

  • Add a set field, which will store the currently ending commands
  • Before every call to command->End, add the command to the set of currently ending commands
  • After we finish ending the command, remove it from the set of currently ending commands
  • Inside Cancel(), check if the command is in the set of currently ending commands- If it is, return
  • Revert the changes to inRunLoop, toSchedule, and toCancel so that they're only to prevent CME/iterator invalidation inside the run loop

Notes:

  • If we don't add the command to the set of currently ending commands when IsFinished() returns true and a command cancels itself in End(), then we'll trigger the interrupt actions and then the finish actions on it. If that's desired behavior, we shouldn't add the command to the set in that case.

@Gold856
Copy link
Contributor Author

Gold856 commented Jul 31, 2023

Using a set works, but I'm still getting that invalid iterator access error on C++, and I don't know how to fix it.

@KangarooKoala
Copy link
Contributor

After tracking the problem to scheduler.Cancel(&cancelDefaultCommand) in the test (by commenting out lines to see what caused the problem) and then to m_impl->scheduledCommands(*find) in CommandScheduler::Cancel (by looking for iterator code in Cancel), it seems that this is the problem: We moved the call to End() between when we get the iterator and when we deference it, so if End() does something that invalidates the pointer- such as schedule a new command- we'll get an error.
The fix is to pass command instead of *find, which also has the benefit of letting us use scheduledCommands.contains() instead of scheduledCommands.find() == scheduledCommands.end().

@Gold856
Copy link
Contributor Author

Gold856 commented Aug 1, 2023

Thank you!

@Gold856
Copy link
Contributor Author

Gold856 commented Aug 1, 2023

/format

@Gold856 Gold856 requested a review from bovlb August 3, 2023 19:19
Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

Besides that nitpick and the cancel() deduplication, should be good to go.

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Looks good to me, except for this one change.

@rzblue rzblue added component: command-based WPILib Command Based Library type: fix Iterations on existing features or infrastructure. labels Sep 18, 2023
@rzblue
Copy link
Member

rzblue commented Sep 18, 2023

This will need conflicts resolved from #5461 - let us know if you need help with that.

@Gold856
Copy link
Contributor Author

Gold856 commented Sep 18, 2023

Squashed everything down and rebased; should be good to go!

Copy link
Member

@rzblue rzblue left a comment

Choose a reason for hiding this comment

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

Looks good to me. We need to keep an eye on this one during beta to ensure no unexpected breakages occur.

Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

Hopefully this won't break anything.

@PeterJohnson PeterJohnson merged commit ff18490 into wpilibsuite:main Oct 10, 2023
24 checks passed
@Gold856 Gold856 deleted the cmd-scheduling-order-fix branch October 10, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: command-based WPILib Command Based Library type: fix Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command unscheduling order is inconsistent
6 participants