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

Alternate approach to SemaphoreStep using Timer #259

Closed
wants to merge 2 commits into from

Conversation

dwnusbaum
Copy link
Member

Alternative to #258 that aims for simplicity over performance. More or less untested, and at the very least it changes some of the logging, so we'd probably want to run the PCT against this first if we want to try this approach.

Comment on lines -148 to -155
/** @deprecated should not be needed */
@Deprecated
public StepContext getContext() {
State s = State.get();
synchronized (s) {
return getContext(s, k());
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Could be restored if needed, but I didn't see any callers in a quick search, and IDK how it would even be used in the first place. Deleting this though allows us to totally drop State.contexts.

@Override public String getStatus() {
State s = State.get();
synchronized (s) {
return "waiting on " + k;
Copy link
Member Author

Choose a reason for hiding this comment

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

Message is less precise now, could be restored if needed.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

This one makes me nervous; adding a background thread seems like it would just increase the chances of nondeterministic test failures.

@dwnusbaum dwnusbaum closed this Mar 29, 2024
@dwnusbaum dwnusbaum deleted the synchronization-fix-alt branch March 29, 2024 17:10
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.

2 participants