rfc42: add credit based flow control for channels

[garlick]

Problem: rfc42 does not specify anything about channel flow control, so it is difficult to design clients and servers that operate without dropping data.

Add credit based flow control to the protocol.

The client receives credit for writes to the subprocess via the exec add-credit response.

The server receives credit for output to the client via the add-credit RPC.

[chu11]

just so i don't forget later, gotta add a "forwarding flag" for credits here

  .. object:: flags                                                                                                                         
                                                                                                                                            
    (*integer*, REQUIRED) A bitfield comprised of zero or more flags:                                                                       
                                                                                                                                            
    stdout (1)                                                                                                                              
      Forward standard output to the client.                                                                                                
                                                                                                                                            
    stderr (2)                                                                                                                              
      Forward standard error to the client.                                                                                                 
                                                                                                                                            
    channel (4)                                                                                                                             
      Forward auxiliary channel output to the client.                                                                                       

[chu11]

in my prototyping I've determined there's two "styles" of using credits

• the user doesn't want to write anything to stdin/channel until it receives its initial credits. Will likely write to stdin via the "credit callback".
• The other style is that the user wants to write data right away without waiting receiving credits. This makes sense b/c initial credits are known ahead of time. So we do not want to send the subprocess to send initial credits.

This will necessitate sending a flag to the remote subprocess to tell it to send/not-send the initial credits.

Any opinions on how to add this to the exec request.

A) new flag that specifies subprocess flags (could also add NO_SETPGRP, FORK_EXEC, etc. flags in there)
B) add it into to the current flags we have here.

My anal retentiveness likes to keep this stuff separate and wants to do 'A'. But that is more bytes in the payload.

[garlick]

It seems like the server should always send credits to the client and the client should wait until it gets that response to send any data. What I was on about earlier was that the server should be able to send that response immediately rather than wait for the local reactor loop to tell it the initial value, since the initial value should already be known to the server.

It would be worrisome to have the client make any assumptions about the size of the server's buffer IMHO.

[chu11]

What I was on about earlier was that the server should be able to send that response immediately rather than wait for the local reactor loop to tell it the initial value, since the initial value should already be known to the server.

So for a remote subprocess I did have to add a flag for the server to tell its local subprocess not to send the "initial credits", since the server can handle that themselves.

It would be worrisome to have the client make any assumptions about the size of the server's buffer IMHO.

We could certainly leave this off until a time it becomes necessary. Atleast w/ my flux-exec prototype, this style was not needed.

[garlick]

Maybe we should establish a minimum buffer size of 4K or so and say that if a subprocess client wants to ignore credits (not register the callback) they can behave as before, but if they send more than 4K of data, it will not be reliable?

I hate to make every client register a credit callback.

[chu11]

I hate to make every client register a credit callback.

They way I initially implemented things, a client does require credits to call flux_subprocess_write(), it's basically used as "information" for the client. They can choose to ignore it if they want and the API is just like it was before. If they don't specify the credit callback, everything stays the same in the API as it was before.

[chu11]

the rune to allow me to push to this branch probably isn't setup, but this is so tiny, perhaps just easiest to manually add this to PR?

diff --git a/spec_42.rst b/spec_42.rst
index ed4d649..37412ab 100644
--- a/spec_42.rst
+++ b/spec_42.rst
@@ -123,6 +123,9 @@ are defined as follows:
     channel (4)
       Forward auxiliary channel output to the client.
 
+    credit (8)
+      Forward channel credits to the client.
+
 Several response types are distinguished by the type key:
 
 .. object:: exec started response

[garlick]

Oh so that's how you got the sdexec tests to pass :-)

This seems reasonable though there may need to be some text added to say what happens with and without this flag. I'll play with it.

[garlick]

Playing devils advocate: is there a reason not to keep it simple and always send the add-credit responses, and let the client ignore them if it doesn't care rather than requiring it to declare its intentions with a flag?

[garlick]

OK, I added the flag renaming it to write-credit (anticipating a flag for reads).

I also got rid of the read direction language since we are not there yet.

Finally, I reworded some of the text concerning credits in the write description, indicating that credit flow control is optional.

[chu11]

Playing devils advocate: is there a reason not to keep it simple and always send the add-credit responses, and let the client ignore them if it doesn't care rather than requiring it to declare its intentions with a flag?

It's a lot of processing and extra traffic that serves no purpose. I could argue why not do that with the output too? If the caller doesn't want the stdout just drop it?

[chu11]

this LGTM given flux-framework/flux-core#6353 work. Not approving at the moment, we probably want to hold off merging until we review that PR a little bit more.

oops, meant just a comment

[garlick]

It's a lot of processing and extra traffic that serves no purpose. I could argue why not do that with the output too? If the caller doesn't want the stdout just drop it?

This allows us to reject an sdexec request with this flag for now, which is appropriate while it's unsupported. That seems like a good enough reason to me for :-) If that weren't the case I'd probably argue that you ignore these messages at your peril, unless you're not using stdin or you know you are writing less than the buffer size, in which case it won't be very many messages wasted.

[garlick]

Just pushed the following changes

• moved input flow control description to a subsection under the write RPC
• described borrowing credit before the initial add-credit response is received
• add JSON example add-credit response
• stated a minimum default input buffer size (4K), so at least that amount can be borrowed if the channel buffer size is not requested to be something different. The actual buffer size can still be less than that (useful in test) on request.

[chu11]

LGTM. Only thing is do we want to say buffer size shall be >= 4096 in the general sense (i.e. input & output). Or we can deal with that tweak when we begin to implement the stdout/output side of this.

[garlick]

Thanks! I vote we cross that bridge when we come to it. Setting MWP.