Move SemaphoreStep states into a single map to try to simplify the implementation
#260
Conversation
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
| sync = false; | ||
| } | ||
| synchronized (s) { | ||
| s.started.add(k); | ||
| // Even if we completed immediately, set the state to StartedState so waitForSuccess knows to stop waiting. |
There was a problem hiding this comment.
Why? I would expect the wait to just wait for the key to be present, with any value.
Also did you mean
| // Even if we completed immediately, set the state to StartedState so waitForSuccess knows to stop waiting. | |
| // Even if we completed immediately, set the state to StartedState so waitForStart knows to stop waiting. |
?
There was a problem hiding this comment.
Because otherwise if you call SemaphoreStep.success or SemaphoreStep.failure and then SemaphoreStep.waitForStart without even running a Pipeline that uses the semaphore step, waitForStart will complete immediately.
EDIT: In practice this should not really happen, but if you had a complex test with multiple threads, it's possible, and the old code did not have that behavior, and it doesn't really seem like a behavior that we would want to introduce now.
There was a problem hiding this comment.
Basically, to really get this right, there needs to be at least 5 states, and we need to add a few more transitions that currently do not exist:
- No entry in the map
- ImmediateFailure (the step will fail, if/when it starts)
- ImmediateSuccess (the step will succeed, if/when it starts)
- Started/Waiting/Blocked (the step has started, and has not yet completed)
- Finished (the step has finished, with some result) (does not currently exist, we'd need to add various transitions to this state)
And I do not think that we can introduce the Finished state without also introducing various behavior changes in edge cases that might affect existing tests. We'd definitely need a PCT run.
There was a problem hiding this comment.
7ffaf48 introduces FinishedState, which I think makes things clearer.
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
We should do a PCT run for any proposed change to this class. ( |
| @@ -108,12 +108,15 @@ public static void success(String k, Object returnValue) { | |||
| StepContext c; | |||
| synchronized (s) { | |||
| KeyState keyState = s.keyStates.get(k); | |||
| if (!(keyState instanceof StartedState)) { | |||
| if (keyState == null || keyState instanceof WillSucceedState || keyState instanceof WillFailState) { | |||
There was a problem hiding this comment.
Maybe introduce a common supertype WillFinishState.
| return; | ||
| } else if (keyState instanceof FinishedState) { | ||
| throw new IllegalStateException(k + " already finished"); |
There was a problem hiding this comment.
This breaks tests. In particular at least workflow-step-api / org.jenkinsci.plugins.workflow.steps.GeneralNonBlockingStepExecutionTest.stop and workflow-cps / org.jenkinsci.plugins.workflow.cps.CpsFlowExecutionTest.stepsAreStoppedWhenCpsVmExecutorServiceHandlesUncaughtException. I'll take a look.
There was a problem hiding this comment.
GeneralNonBlockingStepExecutionTest.stop I think just has a redundant call to SemaphoreStep.success that could be removed, but for compatibility we could replace this exception with just a logged warning.
CpsFlowExecutionTest.stepsAreStoppedWhenCpsVmExecutorServiceHandlesUncaughtException is a little unique, basically the test was using semaphore as a way to call StepContext.onSuccess after a Pipeline dies. To preserve that test's behavior without changes, we'd have to make FinishedState keep the serialized context around. Probably clearer to update that test to use a custom step instead.
See #258 (comment).
Personally, I feel like this is still pretty comparable to #258 in terms of complexity. It seems a little better in terms of synchronization complexity, but the state transition complexity seems about the same or worse. The issue is that the four maps did not represent distinct non-overlapping states and so it is still not that clear what is going on in the new code. If we are purely going for simplicity, something like #259 would be my vote, then #258, then this PR.
If we want to go with this approach, I think this PR would need a PCT run because it seems possible that is accidentally broke some typical use case.