fn: improvements for GoroutineManager #9270
Conversation
Make sure there is no race condition between Done() and Wait() methods in the GoroutineManager implementation. See lightningnetwork#9141 (comment)
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fn/goroutine_manager.go
Outdated
| // Go starts a new goroutine if the manager is not stopping and returns if it | ||
| // started a new goroutine. |
There was a problem hiding this comment.
Nit: Go tries to start a new goroutine and returns a boolean indicating its success ?
There was a problem hiding this comment.
Fixed. I added a note about when it succeeds.
// Go tries to start a new goroutine and returns a boolean indicating its
// success. It fails iff the goroutine manager is stopping or its context passed
// to NewGoroutineManager has expired.
func (g *GoroutineManager) Go(f func(ctx context.Context)) bool {| // TestGoroutineManagerStopsStress launches many Stop() calls in parallel with a | ||
| // task exiting. It attempts to catch a race condition between wg.Done() and | ||
| // wg.Wait() calls. According to documentation of wg.Wait() this is acceptable, | ||
| // therefore this test passes even with -race. |
There was a problem hiding this comment.
Given the implementation of WaitGroups I think we don't need this Stresstest anymore or ? There is no problem in calling wg.Done()and potentially reaching a counter of 0. Only adding new Goroutines are a problem and cause potential race conditions.
There was a problem hiding this comment.
I would keep it as a safeguard in case we change the implementation of GoroutineManager in the future, and the new implementation is not resilient to this scenario.
This was requested in lightningnetwork#9140 (comment)
starius
force-pushed
the
goroutine-manager-bool
branch
from
9665385 to
891e962
Compare
|
Going to merge this and push a tag |
Change Description
Method
Go()was changed to return a (success) bool instead of an error. This is needed to simplify handling of its result, since only ErrStopping could be returned, not any other error. See #9140 (comment)Also test TestGoroutineManagerStopsStress was added to make sure there is no race condition between Done() and Wait() methods in the GoroutineManager implementation. See #9141 (comment)
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.