Using water shared runtime#281
Conversation
Build a single waterv1.SharedDialer per Outbound in loadConfig (compiles the WASM module once) and reuse it for every dial; each dial only instantiates a fresh guest instance, eliminating the per-dial CompileModule that leaked the interpreter's compiled functions (unbounded, multi-GB) and re-decoded the binary every connection. newDialer becomes dialConfig (returns the per-dial config); dialTCP dials via the shared dialer; Close closes it. Also: optional compiler engine via UseCompiler (defaults to interpreter for the NetworkExtension jetsam limit); shallow-copy the per-dial config; close the conn on stream end since water only auto-closes on worker exit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ternd/water-shared-runtime
There was a problem hiding this comment.
Pull request overview
This PR switches the WATER outbound implementation to use the v1 shared runtime/dialer so the WASM module is compiled once and reused across dials, and adds an option to enable wazero’s optimizing compiler on hosts where it’s safe.
Changes:
- Build and retain a
waterv1.SharedDialerduring background config load, and use it for all subsequent TCP dials. - Add
use_compilerto WATER outbound options to toggle compiler vs interpreter runtime configuration. - Bump the
github.com/getlantern/waterreplacement to a newer revision.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| protocol/water/outbound.go | Introduces shared WATER runtime/dialer lifecycle and updates per-dial config + dialing logic. |
| option/water.go | Adds UseCompiler option for choosing compiler vs interpreter. |
| go.mod | Updates the github.com/refraction-networking/water replacement target revision. |
| go.sum | Updates module checksums for the bumped water revision. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The detached dial (context.WithoutCancel) ignores caller cancellation, so an already-canceled caller paid for a full dial before the post-dial check noticed. Short-circuit on ctx.Err() up front. When cancellation is detected after a successful dial, dispatch rawConn.Close() to a goroutine since WATER's Close can block and would otherwise stall the cancellation path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ternd/water-shared-runtime
📝 WalkthroughWalkthroughUpdates the ChangesWATER Shared Dialer Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protocol/water/outbound.go (1)
269-276: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove blocking cleanup outside
o.muand return close errors.
sharedDialer.Closeandseeder.Closecan block while all dial/load paths wait ono.mu;sharedDialer.Closeerrors are also discarded.Proposed refactor
func (o *Outbound) Close() error { o.cancelLoad() o.mu.Lock() - defer o.mu.Unlock() - if o.sharedDialer != nil { - _ = o.sharedDialer.Close(context.Background()) - o.sharedDialer = nil - } - if o.seeder != nil { - return o.seeder.Close() - } - return nil + sharedDialer := o.sharedDialer + o.sharedDialer = nil + seeder := o.seeder + o.seeder = nil + o.mu.Unlock() + + var err error + if sharedDialer != nil { + err = sharedDialer.Close(context.Background()) + } + if seeder != nil { + if seederErr := seeder.Close(); err == nil { + err = seederErr + } + } + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@protocol/water/outbound.go` around lines 269 - 276, The cleanup in the outbound close path holds o.mu while calling sharedDialer.Close and seeder.Close, which can block other dial/load operations, and it also ignores sharedDialer.Close errors. Refactor the close logic in the outbound Close method to first detach sharedDialer and seeder under o.mu, then perform the potentially blocking Close calls after releasing the lock, and make sure any close error from sharedDialer.Close is returned instead of discarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@protocol/water/outbound.go`:
- Around line 396-399: The detached dial in sharedDialer.DialContext currently
drops both caller cancellation and the caller deadline, so reapply the deadline
after context.WithoutCancel(ctx) instead of using it alone. Update the outbound
dial logic in the context-setup around rawConn/sharedDialer.DialContext so the
dial still ignores cancellation but preserves ctx’s timeout/deadline semantics.
---
Nitpick comments:
In `@protocol/water/outbound.go`:
- Around line 269-276: The cleanup in the outbound close path holds o.mu while
calling sharedDialer.Close and seeder.Close, which can block other dial/load
operations, and it also ignores sharedDialer.Close errors. Refactor the close
logic in the outbound Close method to first detach sharedDialer and seeder under
o.mu, then perform the potentially blocking Close calls after releasing the
lock, and make sure any close error from sharedDialer.Close is returned instead
of discarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c75c99-7fbf-40f9-954f-eb7a1654dc5b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modoption/water.goprotocol/water/outbound.go
| // Detach from sing-box's dial-ctx cancellation so the dial isn't torn down | ||
| // mid-flight; since it then ignores caller cancellation, honor it manually | ||
| // below. | ||
| rawConn, err = sharedDialer.DialContext(context.WithoutCancel(ctx), cfg, N.NetworkTCP, "localhost:0") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section
sed -n '340,430p' protocol/water/outbound.go
# Find the shared dialer and related context handling
rg -n "sharedDialer\.DialContext|NetworkDialerFunc|WithoutCancel|WithDeadline|Deadline" protocol -S
# Map the file structure for the relevant file
ast-grep outline protocol/water/outbound.go --view expandedRepository: getlantern/lantern-box
Length of output: 1967
🌐 Web query:
Go context.WithoutCancel removes deadlines documentation
💡 Result:
Yes, context.WithoutCancel removes both the cancellation signal and the deadline from the parent context [1][2]. According to the official Go documentation, context.WithoutCancel (introduced in Go 1.21) returns a derived context that points to the parent but is not canceled when the parent is canceled [1][3]. As part of this contract, the returned context explicitly returns no deadline (Deadline returns ok=false) and no error (Err returns nil), and its Done channel is nil [1][4]. By stripping both the cancellation and the deadline, context.WithoutCancel allows a task to outlive the scope of its parent request while still retaining access to request-scoped values [2]. If a specific deadline is required for the detached work, it must be added back explicitly using another constructor like context.WithTimeout or context.WithDeadline [2].
Citations:
- 1: https://pkg.go.dev/context
- 2: https://dev.to/gabrielanhaia/contextwithoutcancel-when-you-need-work-that-outlives-its-caller-453g
- 3: https://go.dev/pkg/context/?m=old
- 4: https://github.com/golang/go/blob/master/src/context/context.go
Preserve the caller deadline when detaching the dial context. context.WithoutCancel(ctx) clears both cancellation and deadline, so this dial can outlive the request timeout unless the deadline is re-applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@protocol/water/outbound.go` around lines 396 - 399, The detached dial in
sharedDialer.DialContext currently drops both caller cancellation and the caller
deadline, so reapply the deadline after context.WithoutCancel(ctx) instead of
using it alone. Update the outbound dial logic in the context-setup around
rawConn/sharedDialer.DialContext so the dial still ignores cancellation but
preserves ctx’s timeout/deadline semantics.
This PR use the water shared runtime dialer for the outbound for reducing the high memory consumption and cpu usage
Summary by CodeRabbit
New Features
Bug Fixes
Chores