Replace the MemoryLimit constructor trio with MemoryWatch + options#54
Open
znull wants to merge 11 commits into
Open
Replace the MemoryLimit constructor trio with MemoryWatch + options#54znull wants to merge 11 commits into
znull wants to merge 11 commits into
Conversation
MemoryLimit, MemoryLimitWithObserver, and MemoryObserver were three
positional-argument constructors expressing one concept: watch a stage's
RSS, optionally enforce a limit, optionally log the peak. Collapse them
into a single functional-options constructor:
MemoryWatch(stage, eventHandler, WithMemoryLimit(n), WithPeakUsageLogging())
Calling MemoryWatch with neither option now reports an invalid-usage event
and returns the stage unwrapped, mirroring the non-LimitableStage guard.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous memory monitoring constructor trio with a single functional-options API, MemoryWatch, aligning memory limiting and peak RSS logging under one public entry point.
Changes:
- Adds
MemoryWatch,WithMemoryLimit, andWithPeakUsageLogging. - Consolidates the memory watch loops into one configurable implementation.
- Updates tests to use the new API and adds coverage for missing options.
Show a summary per file
| File | Description |
|---|---|
pipe/memorylimit.go |
Introduces the new functional-options memory watching API and unified watch loop. |
pipe/memorylimit_test.go |
Updates memory limit/observer tests to call MemoryWatch and adds invalid-usage coverage. |
pipe/memorylimit_panic_test.go |
Updates panic behavior test setup to use the new watch configuration. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
Comment on lines
57
to
+60
| // stage is still killed. A panic in any other event-handler call (an | ||
| // RSS-read error) is recovered via StartOptions.PanicHandler and the | ||
| // stage keeps running unmonitored; see StartOptions.PanicHandler. | ||
| func MemoryLimit(stage Stage, byteLimit uint64, eventHandler func(e *Event)) Stage { | ||
| // RSS-read error, or the peak-usage report) is recovered via | ||
| // StartOptions.PanicHandler and the stage keeps running unmonitored; see | ||
| // StartOptions.PanicHandler. |
For now it only has one method, which is a `memoryWatchFunc`.
Extract the following methods from `memoryWatcher.watch()`: * `handleGetRSSError()` * `killStage()` * `reportPeakUsage()`
Extract method `memoryWatcher.update()` from `memoryWatcher.watch()`.
Build the stage into the `memoryWatcher`, so it doesn't need to be passed around as an extra argument.
Don't leak a goroutine on panic/recover
Per mhagger's review suggestion on #51: rather than injecting a synthetic `watch` closure that panics, make `fakeLimitableStage.GetRSSAnon()` itself panic, and drive the test through the real `MemoryWatch` constructor and monitor goroutine. This exercises the genuine panic-propagation path. Co-authored-by: Michael Haggerty <mhagger@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per mhagger's review suggestion on #51: replace the re-exec'd subprocess test (and its child-process scaffolding) with a synchronous assert.PanicsWithValue. A panic in the monitor goroutine can't be observed from the test goroutine, so we drive the watch loop directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MemoryLimit, MemoryLimitWithObserver, and MemoryObserver were three positional-argument constructors expressing one concept: watch a stage's RSS, optionally enforce a limit, optionally log the peak. Collapse them into a single functional-options constructor:
Calling MemoryWatch with neither option now reports an invalid-usage event and returns the stage unwrapped, mirroring the non-LimitableStage guard.
/cc @mhagger