fn: expand the ContextGuard and add tests by ellemouton · Pull Request #9343 · lightningnetwork/lnd

ellemouton · 2024-12-10T08:29:12Z

In this commit, the ContextGuard struct is re-worked such that the context that its new main WithCtx method provides is cancelled in sync with a parent context being cancelled or with it's quit channel being cancelled. Tests are added to assert the behaviour. In order for the close of the quit channel to be consistent with the cancelling of the derived context, the quit channel must be contained internal to the ContextGuard so that callers are only able to close the channel via the exposed Quit method which will then take care to first cancel any derived context that depend on the quit channel before returning.

Furthermore, the main WithCtx method, is extended to take a parent context such that any derived context extends the
parent instead of needing to create a fresh one.

PR updating to the new fn version & using the updated ContextGuard: #9344

coderabbitai · 2024-12-10T08:29:20Z

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-10T08:29:54Z

cc @guggero - interested to hear what you think of this 👀

ellemouton · 2024-12-10T12:14:06Z

See usage here: #9344

guggero

Concept ACK, nice improvements! Will make it easier and more clear to use.

guggero · 2024-12-11T08:51:57Z

fn/context_guard.go

+
+// WithCtx is used to derive a cancellable context from the parent. Various
+// options can be provided to configure the behaviour of the derived context.
+func (g *ContextGuard) WithCtx(ctx context.Context,


We could rename this to Create() since this is the main purpose of the struct. Then we can keep the With...() naming for the options above (so WithTimeout(), WithCustomTimeout(), WithBlocking() and so on).

happy to change it to Create

but for the options: the reason I went with TimeoutCGOpt() etc, (ie, usingCGOpt) is mainly cause this isnt in its own package so if we just had WithTimeout but then later on another helper in the fn package introduced functional options where one of them adds a timeout, then what do we call that one? so thought maybe better to namespace from the get go. Thoughts?

Seen this comment now, see my proposal (postfixing with ContextGuard or CG.

cool yeah - went with that 👍

fn/context_guard.go

ellemouton

Thanks for taking a look @guggero 🙏 - updated. Just one question about the naming suggestion

ellemouton · 2024-12-11T16:01:52Z

fn/context_guard.go

+
+// WithCtx is used to derive a cancellable context from the parent. Various
+// options can be provided to configure the behaviour of the derived context.
+func (g *ContextGuard) WithCtx(ctx context.Context,


happy to change it to Create

but for the options: the reason I went with TimeoutCGOpt() etc, (ie, usingCGOpt) is mainly cause this isnt in its own package so if we just had WithTimeout but then later on another helper in the fn package introduced functional options where one of them adds a timeout, then what do we call that one? so thought maybe better to namespace from the get go. Thoughts?

fn/context_guard.go

bhandras

LGTM, very nice work! ✨

bhandras · 2024-12-12T15:44:46Z

fn/context_guard.go

-func (g *ContextGuard) CtxBlocking() (context.Context, func()) {
-	return g.CtxBlockingCustomTimeout(g.DefaultTimeout)
+// important tasks.
+func BlockingCGOpt() ContextGuardOption {


nit: naming wise should we use the With prefix for functional options? So something like: WithBlockingContextGuard/CG, WithCustomTimeoutContextGuard/CG etc? I think this style is widely used around the codebase.

cool yeah good naming suggestion 🙏 updated to this!

bhandras · 2024-12-12T15:46:52Z

fn/context_guard.go

+
+// WithCtx is used to derive a cancellable context from the parent. Various
+// options can be provided to configure the behaviour of the derived context.
+func (g *ContextGuard) WithCtx(ctx context.Context,


Seen this comment now, see my proposal (postfixing with ContextGuard or CG.

fn/context_guard.go

bhandras · 2024-12-12T15:50:12Z

fn/context_guard.go

+			return
+		}
+
+		fn()


Does the cancel fn need to be called while we hold the mutex?

cool - restructured it a bit (still want the delete to fall under the lock)

but fwiw, i dont think a context.CancelFunc is ever blocking so dont think it really matters thaaaat much. updated non the less!

fn/context_guard.go

fn/context_guard_test.go

bhandras · 2024-12-12T16:07:50Z

fn/context_guard_test.go

+
+		// Give the goroutine above a chance to spin up so that it's
+		// waiting on the select.
+		time.Sleep(time.Millisecond * 200)


nit: alternatively you could read from a temp channel that is written to in the goroutine's body.

(and below for similar sleeps).

if we do that then yes we know that the goroutine has at least started, but we dont actually know that it is hanging on the select so it will still be flaky in terms of timing there

fn/context_guard_test.go

bhandras · 2024-12-12T16:16:58Z

fn/context_guard_test.go

+	// will be not be cancelled when the quit channel is closed but will be
+	// if either the context is cancelled or the timeout is reached.
+	t.Run("Blocking context with timeout", func(t *testing.T) {
+		t.Parallel()


I wonder if you'd consider adding a GuardTimeout to these tests (see here:

lnd/cluster/etcd_elector_test.go

Line 19 in fdb43c0

func GuardTimeout(t *testing.T, timeout time.Duration) func() {

) which ensures that we don't timeout and if so it panics and dumps goroutines to the standard out. We use it quite extensively in loop and its very handy.

oh cool! TIL!
I like it!

ok i've added it to the main test.. do you think it should instead be invoked for each sub test?

ellemouton

updated, thanks @bhandras !

ellemouton · 2024-12-13T08:08:24Z

fn/context_guard.go

-func (g *ContextGuard) CtxBlocking() (context.Context, func()) {
-	return g.CtxBlockingCustomTimeout(g.DefaultTimeout)
+// important tasks.
+func BlockingCGOpt() ContextGuardOption {


cool yeah good naming suggestion 🙏 updated to this!

ellemouton · 2024-12-13T08:08:47Z

fn/context_guard.go

+
+// WithCtx is used to derive a cancellable context from the parent. Various
+// options can be provided to configure the behaviour of the derived context.
+func (g *ContextGuard) WithCtx(ctx context.Context,


cool yeah - went with that 👍

fn/context_guard.go

ellemouton · 2024-12-13T08:21:53Z

fn/context_guard.go

+			return
+		}
+
+		fn()


cool - restructured it a bit (still want the delete to fall under the lock)

but fwiw, i dont think a context.CancelFunc is ever blocking so dont think it really matters thaaaat much. updated non the less!

fn/context_guard_test.go

ellemouton · 2024-12-13T08:24:13Z

fn/context_guard_test.go

+
+		// Give the goroutine above a chance to spin up so that it's
+		// waiting on the select.
+		time.Sleep(time.Millisecond * 200)


if we do that then yes we know that the goroutine has at least started, but we dont actually know that it is hanging on the select so it will still be flaky in terms of timing there

ellemouton · 2024-12-13T08:26:25Z

fn/context_guard_test.go

+	// will be not be cancelled when the quit channel is closed but will be
+	// if either the context is cancelled or the timeout is reached.
+	t.Run("Blocking context with timeout", func(t *testing.T) {
+		t.Parallel()


oh cool! TIL!
I like it!

ellemouton · 2024-12-13T08:30:14Z

fn/context_guard_test.go

+	// will be not be cancelled when the quit channel is closed but will be
+	// if either the context is cancelled or the timeout is reached.
+	t.Run("Blocking context with timeout", func(t *testing.T) {
+		t.Parallel()


ok i've added it to the main test.. do you think it should instead be invoked for each sub test?

Copied from lightninglabs/loop

In this commit, the ContextGuard struct is re-worked such that the context that its new main WithCtx method provides is cancelled in sync with a parent context being cancelled or with it's quit channel being cancelled. Tests are added to assert the behaviour. In order for the close of the quit channel to be consistent with the cancelling of the derived context, the quit channel _must_ be contained internal to the ContextGuard so that callers are only able to close the channel via the exposed Quit method which will then take care to first cancel any derived context that depend on the quit channel before returning.

guggero

Nice, LGTM 🎉

guggero · 2024-12-13T09:27:16Z

Pushed a new tag fn/v2.0.5 for this.

guggero self-requested a review December 10, 2024 09:22

ellemouton mentioned this pull request Dec 10, 2024

htlcswitch+go.mod: use updated fn.ContextGuard #9344

Merged

guggero reviewed Dec 11, 2024

View reviewed changes

ellemouton force-pushed the contextGuard branch 2 times, most recently from 1fe1087 to 8bec5f6 Compare December 11, 2024 16:09

ellemouton commented Dec 11, 2024

View reviewed changes

ellemouton added the no-itest label Dec 11, 2024

guggero self-requested a review December 11, 2024 16:54

ellemouton force-pushed the contextGuard branch from 8bec5f6 to b24241f Compare December 12, 2024 10:47

ellemouton mentioned this pull request Dec 12, 2024

Add Outbound Remote Signer implementation #8754

Open

saubyk requested a review from bhandras December 12, 2024 15:21

bhandras approved these changes Dec 12, 2024

View reviewed changes

ellemouton force-pushed the contextGuard branch from b24241f to 8dd07dc Compare December 13, 2024 08:41

ellemouton commented Dec 13, 2024

View reviewed changes

ellemouton added 2 commits December 13, 2024 10:44

fn: add Guard test helper

c2165c4

Copied from lightninglabs/loop

ellemouton force-pushed the contextGuard branch from 8dd07dc to f99cabf Compare December 13, 2024 08:44

guggero approved these changes Dec 13, 2024

View reviewed changes

guggero merged commit ff14847 into lightningnetwork:master Dec 13, 2024
20 of 21 checks passed

ellemouton deleted the contextGuard branch December 13, 2024 09:27

+              			return
+              		}
+              		fn()

Conversation

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

Uh oh!

Uh oh!

coderabbitai bot commented Dec 10, 2024

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 10, 2024

Uh oh!

ellemouton commented Dec 10, 2024

Uh oh!

guggero 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!

Uh oh!

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bhandras 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton commented Dec 10, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)