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

fix: Pass ctx.props to default handlers #3465

Merged
merged 2 commits into from
Feb 25, 2025
Merged

Conversation

tewaro
Copy link
Contributor

@tewaro tewaro commented Feb 5, 2025

No description provided.

@tewaro tewaro requested review from a team as code owners February 5, 2025 06:55
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from 7bede3c to 06dc9cc Compare February 5, 2025 18:33
@tewaro tewaro requested a review from kentonv February 5, 2025 18:37
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch 2 times, most recently from f874f4b to c26f53f Compare February 6, 2025 08:02
Copy link

github-actions bot commented Feb 6, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from c26f53f to f607315 Compare February 6, 2025 14:51
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from f607315 to 7c05d79 Compare February 6, 2025 14:58
@jasnell
Copy link
Member

jasnell commented Feb 6, 2025

What exactly is the intent here? The PR description needs some explanation and detail. If the idea is to give every invocation it's own unique context, then rather than cloning the entire thing the way this PR does, could it make sense to simply wrap the context in a JS Proxy object for every request and either CoW the underlying context or just intercept mutations so that the original remains unmodified? The per-request props can then be injected via the proxy. Wrapping in a proxy could likely reduce much of the complexity here.

@kentonv
Copy link
Member

kentonv commented Feb 6, 2025

What exactly is the intent here? The PR description needs some explanation and detail.

The goal of the change is to fix a known bug in which ctx.props is not available for non-class-based entrypoints because such entrypoints have always reused the same ctx object for all invocations. There's a comment on line 1674 describing the issue:

                        // TODO(cleanup): Unfortunately, for non-class-based handlers, we have
                        //   always created only a single `ctx` object and reused it for all
                        //   requests. This is weird and obviously wrong but changing it probably
                        //   requires a compat flag. Until then, connection properties will not be
                        //   available for non-class handlers.

BTW @tewaro we should update that comment to something like:

"Historically, non-class-based handlers reused the same ctx object for all requests. This was an accident, but some Workers depend on it. Newer worker with the unique_ctx_per_invocation will allocate a new ctx for every request."

Wrapping in a proxy could likely reduce much of the complexity here.

That sounds way more complicated to me. This change as-is is really not very complex at all. I don't see what benefit a Proxy would provide here.

@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch 2 times, most recently from f87e594 to d3c5a1a Compare February 6, 2025 22:25
@tewaro tewaro requested a review from kentonv February 6, 2025 22:25
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from d3c5a1a to f2d764c Compare February 6, 2025 22:26
@tewaro tewaro requested a review from danlapid February 6, 2025 22:27
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from f2d764c to b53030f Compare February 7, 2025 02:46
@tewaro
Copy link
Contributor Author

tewaro commented Feb 7, 2025

@kentonv how do I force the feature flag on all tests? do I need to modify each one?

@tewaro tewaro requested a review from kentonv February 7, 2025 04:18
@tewaro tewaro changed the title fix: Pass ctx.props to default handlers Draft: fix: Pass ctx.props to default handlers Feb 14, 2025
@tewaro tewaro changed the title Draft: fix: Pass ctx.props to default handlers fix: Pass ctx.props to default handlers Feb 14, 2025
@tewaro tewaro marked this pull request as draft February 14, 2025 16:15
@tewaro
Copy link
Contributor Author

tewaro commented Feb 14, 2025

The above and below commits are to show that all tests pass with the change.
Edit: The feature flag will be added back in after they have passed.

@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from 5786aa4 to 04f8e14 Compare February 14, 2025 17:22
@danlapid
Copy link
Collaborator

This should still have a unit test though.

@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch 2 times, most recently from 0a676be to 6fd674c Compare February 21, 2025 07:40
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from 6fd674c to f14be28 Compare February 23, 2025 20:25
@tewaro
Copy link
Contributor Author

tewaro commented Feb 23, 2025

Unit tests added.
Commit f14be28 shows that all tests pass with the new behavior.
Commit f0e7b21 adds back the compatability flags checks.

@tewaro tewaro requested a review from danlapid February 23, 2025 22:54
@tewaro tewaro marked this pull request as ready for review February 23, 2025 22:55
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from f0e7b21 to 3c7b7bb Compare February 24, 2025 15:09
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch 2 times, most recently from 87f0d95 to bc32f54 Compare February 24, 2025 20:25
@tewaro tewaro force-pushed the tewaro/ctx-props-on-default branch from bc32f54 to e1e5e5c Compare February 25, 2025 00:41
@kentonv
Copy link
Member

kentonv commented Feb 25, 2025

Internal build failure looks like a flake.

@tewaro tewaro merged commit a61fffe into main Feb 25, 2025
17 checks passed
@tewaro tewaro deleted the tewaro/ctx-props-on-default branch February 25, 2025 02:29
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