fn: Remove ctx from GoroutineManager constructor by ellemouton · Pull Request #9341 · lightningnetwork/lnd

ellemouton · 2024-12-09T15:37:21Z

Updates the GoroutineManager to avoid having its constructor take
a context since this is an anti-pattern that I think we should avoid. The GoroutineManagers will
generally be created within LND subserver constructors and these will not really ever take contexts
and we want to avoid passing a fresh context to these

this PR was opened in response to reviewing this PR

coderabbitai · 2024-12-09T15:37:30Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

ellemouton · 2024-12-09T16:34:10Z

cc @starius

starius

I like the change in API 🎉

IIUC this change would double the number of goroutines. I proposed one approach how to solve this.

starius · 2024-12-10T02:51:12Z

fn/goroutine_manager.go

+
 	// Calling wg.Add(1) and wg.Wait() when wg's counter is 0 is a race
-	// condition, since it is not clear should Wait() block or not. This
+	// condition, since it is not clear if should Wait() block or not. This


nit: s/if should Wait() block/if Wait() should block/

starius · 2024-12-10T02:57:48Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


This update changes the behavior of Stop(). Previously, if one goroutine called Stop() and another goroutine invoked Stop() concurrently, the second call would block, waiting for the first call to complete. Now, the second Stop() call returns immediately.

is that a problem? is there a usecase for needing to support the blocking behaviour? afaiu, this will mostly be used within other sub-systems and Stop will be called by their Stop methods which typically will also only be called once

In the previous version Stop() actually did StopAndWait(). If we support multiple Stop() calls, I think they should behave the same way. If someone calls Stop() from multiple places, they are likely to expect that both calls block until all goroutines finish, not only the first call.

I just dont think more than 1 system will ever own the GoroutineManager right?

ie, what makes this Stop different from other Stop methods of other subsystems in LND which use a similar pattern to this?

I just dont think more than 1 system will ever own the GoroutineManager right?

I think, the intuition of Stop() method is that after Stop() call (successfully) returning, all the workers are down. If the second Stop() just returns immediately, this intuition is broken.

Can we panic or return error if a second Stop is detected?

i see there is a Done() method - we could always update this to return a dedicated shutdown channel that is only closed after Wait() in Stop

@bhandras - perhaps a 3rd opinion here just to break the tie would help? 🙏

Option 3 sgtm! (Done with shutdown chan)

cool - pushed a diff with option 3 included 🫡

ah - just realised we cannot do this since callers may wait on Done() inside a goroutine that was started within Go().

Undoing this change

fn/goroutine_manager.go

ellemouton

Thanks @starius - updated as per your suggestion.
I've also removed the ContextWithQuit commit as i've realised that I can just
adapt the existing fn.ContextGuard instead

starius

LGTM! 🎉
I tested it with #9140 - works great, after API changes applied.
I left few notes in the PR. It is not clear what do to if Stop is called multiple times.

docs/release-notes/release-notes-0.19.0.md

fn/goroutine_manager.go

starius · 2024-12-11T13:56:37Z

fn/goroutine_manager.go

+		for _, cancel := range g.cancelFns {
+			cancel()
+		}


I propose to clear the map here, to free memory.

can do, although i dont think it is necessary since this is not a restartable system. This will only be called when the system is shutting down right? if we set the map to nil here, then you could argue that in every Stop method we have in LND, we should go and set each object to nil.

Interested to hear what a second reviewer thinks too. But yeah, can defs add a g.cancelFns = nil line but cant do delete(g.cancelFns, id) since deleting from a map while iterating over it is not safe i think

starius · 2024-12-11T14:47:35Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


In the previous version Stop() actually did StopAndWait(). If we support multiple Stop() calls, I think they should behave the same way. If someone calls Stop() from multiple places, they are likely to expect that both calls block until all goroutines finish, not only the first call.

starius · 2024-12-11T14:52:41Z

fn/goroutine_manager_test.go

-// 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.
-func TestGoroutineManagerStopsStress(t *testing.T) {


We should decide what to do with multiple Stop() calls. If we allow them, we should keep this test to make sure gm doesn't crash in this situation. If we don't allow them, we should panic or return error from Stop is called more than once.

see the other comment re supporting multiple stop calls

ellemouton

Thanks for the review @starius - addressed and replied! Will help to get a second opinion regarding your points for the Stop method

ellemouton · 2024-12-11T15:32:50Z

fn/goroutine_manager.go

+		for _, cancel := range g.cancelFns {
+			cancel()
+		}


can do, although i dont think it is necessary since this is not a restartable system. This will only be called when the system is shutting down right? if we set the map to nil here, then you could argue that in every Stop method we have in LND, we should go and set each object to nil.

Interested to hear what a second reviewer thinks too. But yeah, can defs add a g.cancelFns = nil line but cant do delete(g.cancelFns, id) since deleting from a map while iterating over it is not safe i think

ellemouton · 2024-12-11T15:33:50Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


I just dont think more than 1 system will ever own the GoroutineManager right?

ie, what makes this Stop different from other Stop methods of other subsystems in LND which use a similar pattern to this?

ellemouton · 2024-12-11T15:34:15Z

fn/goroutine_manager_test.go

-// 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.
-func TestGoroutineManagerStopsStress(t *testing.T) {


see the other comment re supporting multiple stop calls

docs/release-notes/release-notes-0.19.0.md

bhandras

LGTM 🌮

fn/goroutine_manager.go

ellemouton

Dankie @bhandras !

fn/goroutine_manager.go

ellemouton · 2024-12-12T06:54:34Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


but why will there ever be a second Stop call is what im getting at. Look at all the instances of ) Stop() { in the codebase

ellemouton · 2024-12-12T06:56:40Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


I think that if we really want to have a method that explicitly blocks and does so for multiple routines, then we should have an explicit Done() <-chan struct method that does this

ellemouton · 2024-12-12T06:58:11Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


i see there is a Done() method - we could always update this to return a dedicated shutdown channel that is only closed after Wait() in Stop

ellemouton · 2024-12-12T06:59:29Z

fn/goroutine_manager.go

+		// Go call starts running here after acquiring the mutex, it
+		// would see that the context has expired and return false
+		// instead of calling wg.Add(1).
+		g.wg.Wait()


@bhandras - perhaps a 3rd opinion here just to break the tie would help? 🙏

bhandras

LGTM 🍫

ellemouton · 2024-12-12T08:05:20Z

Pointing to this change from this PR. So I think we can merge here once the CI is green on that side

ellemouton · 2024-12-12T09:01:24Z

cc @guggero for bypass merge & fn tag pretty please 🙏 (the PR pointing to this is passing CI)

guggero · 2024-12-12T09:03:56Z

Pushed a new tag fn/v2.0.4.

ellemouton mentioned this pull request Dec 9, 2024

htlcswitch: use fn.GoroutineManager #9140

Open

8 tasks

ellemouton changed the title ~~Fn context~~ fn: ContextWithQuit Dec 9, 2024

starius reviewed Dec 10, 2024

View reviewed changes

ellemouton force-pushed the fnContext branch from 0f1388a to 8c19ec0 Compare December 10, 2024 05:08

ellemouton changed the title ~~fn: ContextWithQuit~~ fn: Remove ctx from GoroutineManager constructor Dec 10, 2024

ellemouton commented Dec 10, 2024

View reviewed changes

ellemouton requested a review from starius December 10, 2024 05:39

ellemouton mentioned this pull request Dec 10, 2024

protofsm: update GR Manager usage and start using structured logging #9342

Merged

ellemouton self-assigned this Dec 10, 2024

starius approved these changes Dec 11, 2024

View reviewed changes

ellemouton force-pushed the fnContext branch from 8c19ec0 to 6825bde Compare December 11, 2024 15:36

ellemouton commented Dec 11, 2024

View reviewed changes

ellemouton added the no-itest label Dec 11, 2024

bhandras self-requested a review December 11, 2024 15:56

bhandras approved these changes Dec 11, 2024

View reviewed changes

fn/goroutine_manager.go Show resolved Hide resolved

fn/goroutine_manager.go Outdated Show resolved Hide resolved

fn/goroutine_manager.go Show resolved Hide resolved

ellemouton force-pushed the fnContext branch from 6825bde to bb3442f Compare December 12, 2024 06:58

ellemouton commented Dec 12, 2024

View reviewed changes

ellemouton force-pushed the fnContext branch from bb3442f to 186db3e Compare December 12, 2024 07:13

ellemouton added 2 commits December 12, 2024 09:32

fn: Remove ctx from GoroutineManager constructor

51eeb9e

docs: add release notes entry

37bb082

ellemouton force-pushed the fnContext branch from 186db3e to 37bb082 Compare December 12, 2024 07:33

bhandras approved these changes Dec 12, 2024

View reviewed changes

guggero merged commit e68b2ad into lightningnetwork:master Dec 12, 2024
16 of 19 checks passed

ellemouton deleted the fnContext branch December 12, 2024 09:03

Conversation

ellemouton commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

ellemouton commented Dec 9, 2024

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhandras Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

ellemouton commented Dec 9, 2024 •

edited

Loading

coderabbitai bot commented Dec 9, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

bhandras Dec 12, 2024 •

edited

Loading