Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python: Pass the env parameter into handlers #1610

Merged
merged 15 commits into from
Feb 6, 2024
Merged

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Feb 2, 2024

This allows Python workers to access secrets and such.

This adds a relaxed_call function and uses it to pass the env argument to the fetch/test handlers.
The relaxed_call implementation is from here:
pyodide/pyodide#4392

Before this PR we instantiate the Python interpreter from scratch on every
request. This is slow and uses up a lot of memory. This switches to sharing an
interpreter across requests. This makes per-request non-coldstart
execution time and memory usage much lower.
This adds a relaxed_call function taken from
pyodide/pyodide#4392
and uses it to pass the env argument to the fetch/test handlers
@hoodmane hoodmane requested review from a team as code owners February 2, 2024 21:18
@hoodmane hoodmane requested review from jasnell and kentonv and removed request for a team February 2, 2024 21:18
async fetch(request, env) {
const mainModule = await getMainModule();
return await mainModule.fetch(request);
async fetch(ctx, env) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should rename request to ctx here. With fetch(...) handlers, the third argument is the ExecutionContext that, by convention, we've typically named ctx. Would be good to avoid confusion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about for test? Should the first argument always be called request? And presumably if fetch takes a third argument I should pass that along too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, for test it should be ctx. I'm not a big fan of the difference but that first argument to fetch is a Request object so the name should match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, test's arguments are test(controller, env, ctx). ctx is still the third argument. controller is equivalent to request.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

src/pyodide/python-entrypoint-helper.js Outdated Show resolved Hide resolved
src/pyodide/python-entrypoint-helper.js Outdated Show resolved Hide resolved
Base automatically changed from hoodmane/python-share-interpreter to main February 5, 2024 23:03
@hoodmane hoodmane merged commit 6b63c70 into main Feb 6, 2024
11 checks passed
@hoodmane hoodmane deleted the hoodmane/python-pass-env branch February 6, 2024 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants