-
Notifications
You must be signed in to change notification settings - Fork 578
Fix context in protocol callbacks #348
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
Conversation
My memory here is a little rusty, but can we instead change Lines 338 to 421 in 465717f
|
Maybe yes - they're intended to bridge the native callbacks anyways, right? Submitted a commit to try this approach. I had to hack in the |
I think we need parallel APIs like |
Or add a trailing argument to all |
Got it, that makes sense - I'll then try to propose something more complete. |
/me is trying to fix this one now. |
Nice, what's the latest? ;) |
5f36d64
to
32979f8
Compare
uvloop/handles/handle.pyx
Outdated
| +- UnixServer (pipe) | ||
+- UVTimer (timer) | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks for adding it
70b6ae4
to
81d3900
Compare
6f41c33
to
e9b4056
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good so far!
ab49e7a
to
b41a50d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This is a combined fix to correct contexts from which protocal callbacks are invoked. In short, callbacks like data_received() should always be invoked from consistent contexts which are copied from the context where the underlying UVHandle is created or started. The new test case covers also asyncio, but skipping the failing ones.
6c2d403
to
41844b6
Compare
A quick recap of uvloop core:
Loop
is the uvloop-version of an asyncio event loop, exposing APIs to createUVHandle
s like TCP transports.UVHandle
is the base class of uvloop wrappers of the libuvuv_handle_t
structs, see the full family below.UVHandle
referencesonezero or moreHandle
instances that encapsulate the actual callback, its arguments, and a PEP-567 context.cdef
function perUVHandle
that is registered to the libuvuv_handle_t
struct. This function usually just triggers running theHandle
.UVHandle
usesHandle
for callbacks, and we don't want to change that.What is missing is e.g. thelisten_handle
of someUVHandle
subclasses like theUVStreamServer
. For now, it calls the actual callback (_on_listen
) directly without PEP-567 context. So this PR should add those missingHandle
s.The tricky part is, the currentHandle
supports only fixed parameters provided at initialization, what is needed for callbacks like_on_listen
or__uv_stream_on_read
is a partial-likeHandle._run_with_param()
.List of
UVHandle
s to check:UVAsync
UVCheck
UVIdle
UVPoll
UVProcess
UVProcessTransport
UDPTransport
UVStream
UVStreamServer
UVTimer
Principals
UVHandle
instance (including transports, servers, etc) sticks to one context where it was created from and/or started to take effect.UVHandle
(mostly protocol callbacks, but also protocol factory) runs in the same context.UVHandle
instances share the same context - for example, protocol callbacks are always called in the same context even though the transport is upgraded bystart_tls()
in a different context (even multiple times, a.k.a. SSL over SSL).Context
instance - it can be a copied instance. Therefore, changes to aContextVar
are not always carried between different protocol callbacks. But all callbacks could see the same inherited value from the context where theUVHandle
was created or started.Context.copy()
orcopy_context()
in user-triggered events, and reuse existingContext
instances for uvloop-triggered events, so as to avoid re-entering the same context twice.copy_context()
is only applied to direct method calls, callbacks through e.g.loop.call_soon()
are not considered necessary to copy the context.Example 1: Client Protocol
Expected result:
Example 2: Server Protocol
Expected result:
Example 3:
start_serving
Similar to example 2, but start serving in a different context:
Expected result:
Example 4: TLS Upgrade
Expected result: