[o11y] Fix tail stream cancellation warning #4030
Conversation
There was a problem hiding this comment.
I don't think this is right.
If you are seeing this membrane actually revoke anything, that implies that capabilities associated with the session were still open at the time that sendRpc() finished or was canceled. It's important that capabilities associated with the event don't remain live beyond the lifetime of the event itself. That's exactly what the revoker is enforcing.
Now, it may be true that this revoker pattern isn't really needed: if the caller of sendRpc() always made sure to drop all capabilities before canceling it, and if it only completed normally after all capabilities had been dropped, then there's no need to revoke anything. For many event types, this is a reasonable assumption. JSRPC only needs the revoker because it lets the application send arbitrary capabilities, and since we can't force the application to clean up after itself, we have to forcefully revoke things.
HOWEVER, if the revoker actually is revoking something in your case, that implies that you have some other bug which the revoker is uncovering. Somehow, you're either keeping capabilities open too long, or sendRpc() is completing prematurely, or both. That's the real bug you need to fix -- deleting the revoker doesn't actually fix it.
fhanau
force-pushed
the
felix/041825-tail-worker-cleanup2
branch
from
f6d4cea to
3b77ba6
Compare
|
Managed to fix the cancellation issue for the affected RPC-based test after some debugging and internal discussion. Some open questions remain:
|
@kentonv is the CompletionMembrane still needed here? |
fhanau
force-pushed
the
felix/041825-tail-worker-cleanup2
branch
2 times, most recently
from
373cc1a to
74f36b6
Compare
The revoker membrane warning happened because the revoker membrane was revoked when the client-side capability was deallocated, only doing that when the customEvent is complete resolves this issue. Additionally, draining the tail stream IOContext must be awaited synchronously so that the customEvent returns when it is done, avoiding cancellations.
fhanau
force-pushed
the
felix/041825-tail-worker-cleanup2
branch
from
74f36b6 to
0e91fc9
Compare
fhanau
changed the title
Change how we drain the TailStreamCustomEventImpl incoming request to how it was before #4030 (fixing stream cancellation warning): We believe that this caused drain() to be invoked too late, resulting in "failed to invoke drain()" warnings and possibly some stream cancellation issues.
Change how we drain the TailStreamCustomEventImpl incoming request to how it was before #4030 (fixing stream cancellation warning): This caused drain() to be invoked too late, resulting in "failed to invoke drain()" warnings and stream cancellation issues.
Change how we drain the TailStreamCustomEventImpl incoming request to how it was before #4030 (fixing stream cancellation warning): This caused drain() to be invoked too late, resulting in "failed to invoke drain()" warnings and stream cancellation issues.
The revoker membrane was an artifact of trace-stream being ported from JSRPC and should not be needed here.