refactor: propagate ctx into the four public constructors#146
Merged
Conversation
Finishes the work started in #144. The four public consumer constructors now take ctx as the first argument: - polyscript.New[E](ctx, src, opts...) - extism.FromExtismLoader(ctx, ldr, opts...) - risor.FromRisorLoader(ctx, ldr, opts...) - starlark.FromStarlarkLoader(ctx, ldr, opts...) The three inline context.Background() injections in engines/*/new.go go away; cancellation now actually reaches the loader's I/O and the compiler's parse path. deprecated.go keeps its twelve legacy FromXxx* signatures unchanged and injects context.Background() inline at each downstream call. They are slated for removal in #104; churning their signatures during the deprecation window adds caller pain without value. New cancellation tests: - engines/risor/new_test.go: TestFromRisorLoader_CompileCancelled — pre-cancelled ctx aborts the Risor parser - engines/extism/new_test.go: TestFromExtismLoader_LoaderCancelled — pre-cancelled ctx aborts the loader's GetReader (the wazero compile of the small embedded test module completes too fast to demonstrate parser-level cancellation) - polyscript_test.go: TestNew_CompileCancelled (top-level Risor cancel) and TestNew_HTTPLoaderCancelled (end-to-end: slow httptest server, ctx deadline, verify the fetch aborts near the deadline rather than waiting for the full server stall) Closes #145. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
CI gci lint flagged formatting on the new ctx-injected From*Loader calls. gofumpt prefers each arg on its own line when wrapping; the mechanical sed I used to inject t.Context() left some calls with two args on the wrap line. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the context-threading refactor by adding context.Context as the first parameter to the four public consumer constructors so cancellation can reach loader I/O (e.g., HTTP fetch) and engine compile/parse paths.
Changes:
- Add
ctx context.Contextas the first argument topolyscript.New[E]and the three engine-levelFrom*Loaderconstructors. - Remove inline
context.Background()injection from engine constructors; keep deprecated constructors unchanged by injectingcontext.Background()at call sites. - Add/adjust tests and examples to pass
t.Context()/context.Background()and add cancellation-focused test coverage.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| readme_test.go | Updates README-backed tests to pass t.Context() into polyscript.New. |
| polyscript.go | Changes New[E] signature to accept ctx and threads it into engine construction. |
| polyscript_test.go | Updates call sites and adds end-to-end cancellation tests (compile + HTTP loader). |
| polyscript_options_test.go | Updates option/constructor tests to pass t.Context(). |
| polyscript_extism_test.go | Updates Extism construction tests to pass t.Context(). |
| examples/simple/{risor,starlark,extism}/main.go | Threads a context into polyscript.New in examples. |
| examples/multiple-instantiation/{risor,starlark,extism}/main.go | Threads a context into polyscript.New in examples. |
| examples/data-prep/{risor,starlark,extism}/main.go | Threads a context into polyscript.New in examples. |
| engines/starlark/new.go | Adds ctx to FromStarlarkLoader and forwards it into script.NewExecutableUnit. |
| engines/starlark/new_test.go | Updates Starlark engine constructor tests to pass t.Context(). |
| engines/risor/new.go | Adds ctx to FromRisorLoader and forwards it into script.NewExecutableUnit. |
| engines/risor/new_test.go | Updates Risor engine constructor tests and adds cancellation test. |
| engines/extism/new.go | Adds ctx to FromExtismLoader and forwards it into script.NewExecutableUnit. |
| engines/extism/new_test.go | Updates Extism engine constructor tests and adds loader-cancellation test. |
| engines/integration_test.go | Updates integration tests to pass t.Context() into engine constructors. |
| deprecated.go | Keeps deprecated constructors’ signatures and injects context.Background() at downstream calls. |
| CHANGELOG.md | Documents the breaking change and the new cancellation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.



Summary
Finishes the work started in PR #144. The four public consumer constructors now take
ctxas the first argument:polyscript.New[E](ctx, src, opts...)extism.FromExtismLoader(ctx, ldr, opts...)risor.FromRisorLoader(ctx, ldr, opts...)starlark.FromStarlarkLoader(ctx, ldr, opts...)The three inline
context.Background()injections inengines/{extism,risor,starlark}/new.goare gone. Cancellation now actually reaches the loader's I/O and the compiler's parse path.deprecated.go
The twelve legacy
FromXxx*constructors keep their signatures unchanged and injectcontext.Background()inline at each downstream call toFrom*Loader. They are slated for removal in #104; churning their signatures during the deprecation window adds caller pain without value.New tests
engines/risor/new_test.go::TestFromRisorLoader_CompileCancelled— pre-cancelled ctx aborts the Risor parser. Assertserrors.Is(err, context.Canceled).engines/extism/new_test.go::TestFromExtismLoader_LoaderCancelled— pre-cancelled ctx aborts the loader'sGetReader. (Wazero's compile of the small embedded test module completes too fast to reliably demonstrate parser-level cancellation; the loader-IO cancellation path is the meaningful one for end-users anyway.)polyscript_test.go::TestNew_CompileCancelled— top-level Risor cancellation viapolyscript.New[polyscript.Risor].polyscript_test.go::TestNew_HTTPLoaderCancelled— end-to-end demonstration: a stallinghttptest.Server, a 50ms ctx deadline, an assertion that the fetch aborts near the deadline (rather than waiting for the 2s server stall) and returnscontext.DeadlineExceeded. This is the precise scenario [v1][breaking] Propagate ctx into polyscript.New[E] and the three FromXxxLoader constructors #145 set out to fix.Files
polyscript.go,engines/{extism,risor,starlark}/new.go— add ctx as first argdeprecated.go— injectcontext.Background()inline at each callexamples/*/main.go— thread ctx_test.gofiles — passt.Context()at call sitesCHANGELOG.md—[Unreleased] > ChangedNet diff: 23 files changed, 258 insertions, 120 deletions.
Closes #145.
Test plan
go build ./...cleango vet ./...cleango test -race -count=1 ./...full suite greengrep -rn 'context\.Background()' --include='*.go' engines/{extism,risor,starlark}/new.go polyscript.goreturns zero hitsgrep -c 'context\.Background()' deprecated.goreturns 12 (one per legacy constructor)https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code