rfc42: add subprocess attach RPC #479
Conversation
|
|
1 similar comment
|
|
garlick
left a comment
garlick
left a comment
There was a problem hiding this comment.
LGTM, just one phrase I didnt quite understand.
Also just a thought for later on, not a review comment: once attach is supported after an instance restart, we'll want to think about how to handle EOF and stdin writes if we can't reattach to the stdio streams.
spec_42.rst
Outdated
| The :program:`attach` RPC attaches to an existing background subprocess, | ||
| converting it to a streaming subprocess. |
There was a problem hiding this comment.
What is meant by "converting it to a streaming subprocess"?
There was a problem hiding this comment.
This refers to the "Streaming request: " noted in the exec RPC definition (as opposed to non-streaming request which creates a "background" process)
Streaming request: When initiated with a streaming request, responses
(defined below) SHALL be sent until the process terminates and all output
has been delivered. If the requesting process disconnects, the subprocess
SHALL be terminated.
Probably need a better term here. Also, the behavior defined in the additions in this PR leave the attached process running as opposed to killing it on disconnect (seemed like the correct behavior since the subprocess will still have the background flag, and for our intended future use). Happy to take suggestions.
There was a problem hiding this comment.
Hmm 🤔 Well, since the process still behaves as a background process when the attach terminates, it seems like the subprocess isn't really converted per se.
Maybe that part of the sentence ("converting it to a streaming subprocess") should just be dropped?
There was a problem hiding this comment.
Sure, less words is better.
|
Is there a reason this wasn't merged? |
|
Oh, we could merge it. I was just waiting to work on a partial implementation to make sure there were no necessary changes. But changes could just be made in a future PR. |
Add an attach RPC to the subprocess server protocol, which allows a client to attach to an existing background subprocess. Key design decisions: - The attach request includes a flags parameter for future extensibility, but no flags are currently supported. Output forwarding behavior is inherited from the original subprocess. - An "exec attached" response is sent first to indicate success and communicate the subprocess flags to the client, followed by the same response types used for streaming exec requests. - If a waitable subprocess has already terminated, the full response sequence is still sent (attached->output EOFs->finished->ENODATA). - A successful attach that receives the finished response reaps the subprocess, preventing it from being waited on or attached to again. - If the client disconnects, the subprocess reverts to background mode rather than being terminated. Error conditions: - ENOENT: subprocess doesn't exist or has exited without waitable flag - EBUSY: subprocess is not in background mode - EPERM: client lacks authorization
Merge Queue Status✅ The pull request has been merged at 79f83bb This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
|
|
2 participants
This PR adds an attach RPC to the subprocess server protocol, which allows a client to attach to an existing background subprocess and convert it to a streaming subprocess.
This should be considered a first cut - happy to take feedback at this point.
Key design decisions:
The attach request includes a flags parameter for future extensibility, but no flags are currently supported. Output forwarding behavior is inherited from the original subprocess.
An "exec attached" response is sent first to indicate success and communicate the subprocess flags to the client, followed by the same response types used for streaming exec requests.
If a waitable subprocess has already terminated, the full response sequence is still sent (attached->output EOFs->finished->ENODATA).
A successful attach that receives the finished response reaps the subprocess, preventing it from being waited on or attached to again.
If the client disconnects, the subprocess reverts to background mode rather than being terminated.
Error conditions: