Add reset and barrierAction to CyclicBarrier.

[HSAR]

Both reset() and getNumberWaiting() are methods that exist on java.util.concurrent.CyclicBarrier and function in the same way.

This PR comes from discussion on issue #3054.

[HSAR]

I'm opening this PR in the hope of starting a conversation about the implementation and tests, both of which I'm aware could do with some improvement.

[nomisRev]

@HSAR thank you so much for the PR! I need to make some time to properly look at these changes, sorry it's been taking so long but it's not a PR I can quickly review 😅

On first sight it looks pretty good, but I need to dig around a bit and perhaps write some tests for myself. Concurrency is hard 😅

[HSAR]

No problem. Keep me posted if/when you have questions or comments!

[nomisRev]

Hey @HSAR,

There are some tests hanging on CI, and also for me locally. Not sure if you have time to look into it, but I am also taking a look at it whilst reviewing the code.

[HSAR]

Hi @nomisRev - happy to take a look. I had thought that everything was passing when I left it last time, but I am very unfamiliar with this test framework and it is possible that I wasn't running it correctly.

[nomisRev]

@HSAR I think we lost some history due to force-push, because I remember it passing at some time. You should be able to run the tests from IDEA. I think you can ignore checkAll for these tests too if you want. Or write them in Kotlin-test if you're more familiar with that. I'd prefer to have better tests, having some style difference in them is low-tech debt in favour of better tests IMO.

I can always help you refactor them to Kotest if really need to.

[HSAR]

I expected to be able to run the tests from IDEA, but that doesn't seem to be the case (at least, not by default), which rather puzzled me. I'm personally only familiar with JUnit, but I am perfectly happy with any test framework if I can get it to run properly in my IDE. Are there extra steps I need to follow?

Regarding the review, I restructured the code more than once to try and avoid problems with the reset action (which has turned out to be really rather tricky). This one works in many of scenarios but I don't think it is watertight - I am worried for instance about StateMachine::startNextEpoch which seems like it is not nearly atomic enough.

[nomisRev]

Hey @HSAR,

I am taking a stab at it from scratch, to see how far I can get it. That's easier for me to get my head around it 😅 I'll keep you posted today or tomorrow, so we can discuss code in further detail.

To run Kotest you should install the IDEA Kotest plugin, but it sometimes doesn't behave perfectly. I can for example not run individual tests, only entire test files which is kind-of unfortunate. I've contemplated refactoring to JUnit Kotlin-test to leverage things like WASM, but that seems like a too big of an effort for now.

[HSAR]

I might hold off trying to fix the tests, as it seems entirely possible we might junk the current implementation anyway. Happy to chat code if it would help.

[nomisRev]

Hey @HSAR,

I played a little bit around with the implementation and came up with #3076 What do you think? Not sure what the best approach is, because I don't want to throw out your effort. I think you should get credit for leading this in the git history.

[HSAR]

I like the code inside ::reset and the use of a specific CyclicBarrierCancellationException, both are a lot tidier. A few things that do come to mind, though:

• The epoch doesn't advance when a reset happens. That is of course part of the internal logic and state of CyclicBarrier so is acceptable from an external API point of view, I just thought it might be useful to point out. The two State possibilities should, I presume, protect from any thread-unsafe-ness.
• Keeping the barrierAction feature would be quite desirable: my implementation used it as a neat way of implementing the reset but that was not the main reason it existed, which was both because it was a useful feature and because it was part of the original java.util.concurrent CyclicBarrer.

As for the git history, the code is yours and so should be the git blame ;)
Jokes aside I won't lie, it would be nice to be recognised for contribution. Perhaps if you agree with my second point then I can add the barrierAction and thus obtain a little credit if/when all is merged.

[HSAR]

PS. Unused imports at the top of the file ;)

[nomisRev]

Hey @HSAR,
I am closing my other PR. I merged the changes and tried to keep as much as your changes as possible to give you proper credit for driving this effort 🙏 It was not easy 😅

Sorry the entire process was so extraordinarily long.. Please do a thorough review of all my work, and I think with a review from @serras or @franciscodr we should be good to merg this work 🙌

[HSAR]

Hey @nomisRev,

I was a bit confused because I had pushed a different, more-conservative merge not too long ago. LGTM apart from one comment which I've left inline.

I'm glad to have contributed! Thanks for your time looking into the issue and understanding the problems.

[serras]

Nice job!

[nomisRev]

Thank you for the awesome work @HSAR! Congrats on your first contribution 🙌 Very impressive work! 👏

[HSAR]

Happy to have my contribution accepted. Thanks for your part in this too!