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

Support reverse initial N for REQUEST_CHANNEL "fast-start" #249

Open
lexs opened this issue Feb 12, 2018 · 7 comments
Open

Support reverse initial N for REQUEST_CHANNEL "fast-start" #249

lexs opened this issue Feb 12, 2018 · 7 comments

Comments

@lexs
Copy link
Contributor

lexs commented Feb 12, 2018

The REQUEST_CHANNEL frame currently (like all the other request frames) contains the downstream initial N. This allows the server to immediately send down data. However the reverse isn't possible, the client has to wait a full roundtrip to be able to send data as the server must send a REQUEST_N frame to the client, allowing it to send data upstream.

This is problematic when using REQUEST_CHANNEL for anything that is client-driven, for example if you'd want to implement http2 over rsocket (let's play with the idea). Any requests from the client would have to wait a full roundtrip before we can send them, which is unacceptable in terms of latency. We could send a request inline in the REQUEST_CHANNEL payload, but what if we have a request that arrives just after we sent it?

I'm proposing an addition to the protocol spec allowing the REQUEST_CHANNEL to contain a reverse initial N (naming TBD) which instead applies from the server down to the client. The server is free to reject this if wants, or accept it and assume this is now the outstanding credits.

This means the REQUEST_CHANNEL frame would now look something like this:

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                           Stream ID                           |
    +-----------+-+-+-+-+-----------+-------------------------------+
    |Frame Type |0|M|F|C|  Flags    |
    +-------------------------------+-------------------------------+
    |0|                    Initial Request N                        |
    +---------------------------------------------------------------+
    |0|                Initial Reverse Request N                    |
    +---------------------------------------------------------------+
                           Metadata & Request Data

On the client the API would now look something like this:

Flux <Payload> requestChannel(Publisher<Payload> payloads, Options);

and could be used something like:

return requestStream(Flux.just(...), Options.reverseInitialN(5));

Practically on the client this would have the rsocket implementation immediately requestN from the Publisher after the REQUEST_CHANNEL is sent.

This is something we need and are interested in implementing, starting in rsocket-cpp. Starting with an issue for discussion before moving into spec. Spec would either always include this reverse request N or we could use a flag to make it optional.

@stevegury
Copy link
Member

IIRC, we briefly discussed about this during the early phase of design. At the time we decided not to add such field mostly for the sake of simplicity.

However I understand that they are some use pathological use cases where that field may be useful and we should discuss its implications. I think it's definitely possible to add such feature, but before, we need to add some clarification in the spec about the client/server behavior when the server refuse to satisfy the "Initial reverse Request N".

The typical use case will be when a client has multiple messages that need to be sent as soon as possible after establishing the connection, in that case, the server may refuse to process some of them. Multiple options are possible:

  • the server process only a few messages, drop the others and send that information to the client.
  • the server refuses to establish the channel and reject everything.
    In both cases, the client must store all the messages until confirmation that they have been processed.

If the server refuse to establish the channel, what should the client do? Retry with a smaller "Initial reverse Request N" or ask another server? This could lead to potential failure where a client can't find a server able to answer its calls.

@lexs what do you think?

@yschimke
Copy link
Member

yschimke commented Feb 13, 2018

I think this makes a valid Push usecase possible, and to me should go hand in hand with allowing the client to specify 0 as the initial request N. It cleans up this usecase nicely.

@stevegury regarding "This could lead to potential failure where a client can't find a server able to answer its calls."

We can divide use of the protocol into those established "by the spec" where the behaviour needs to be derivably based on the observable properties of the frames. This is similar to the HTTP model with a common protocol but extremely varied client and servers. And a second category of use where the client and server have prior knowledge of each other and should be able to specify strict requirements that suggest something is broken and should be rejected if not met.

I think this internal use case is the latter. A mobile client with limited ability to retain data connects to a presumably much more resource capable server and needs to send data without doing a complicated sequence of round trips to enable. Without this users of the library/protocol will just resort to workarounds like packing multiple frames into a single fake "setup frame" and sending.

In this case the client could do either of your suggestions, or drop the content on the floor and log an error. For the server, the simplest model is ignore everything.

@yschimke
Copy link
Member

yschimke commented Feb 13, 2018

If we are worried about data loss, we already have a model that allows for subscribers with aggressive request infinite and cancel. So this isn't new.

@lexs
Copy link
Contributor Author

lexs commented Feb 13, 2018

I agree we need to define the failure case when the server doesn't accept the "reverse initial N". I think this should behave similarly to how SETUP is rejected, ie there is no real negotiation and we leave it up to the application. We could add a new error code for this to make it clear.

The typical use case will be when a client has multiple messages that need to be sent as soon as possible after establishing the connection, in that case, the server may refuse to process some of them.

I think this is a great reason why "reverse initial N" makes sense, this enforces client and server to agree on the number of messages the server can accept without asking for them explicitly. This means we shouldn't get in a situation where the server has to drop anything.

If the server refuse to establish the channel, what should the client do? Retry with a smaller "Initial reverse Request N" or ask another server? This could lead to potential failure where a client can't find a server able to answer its calls.

I think we'll have to make the wording in the spec very clear that this is entirely up to the application, but practically it seems like a safe fallback is to buffer locally and set "reverse initial N" to 0.

I think there's enough agreement in this thread that I'll start drafting a spec PR (unless somebody is more inclined). Happy for the conversation to continue here meanwhile.

@benjchristensen
Copy link
Contributor

@tmontgomery If you have some time, your input on this would be very helpful.

@robertroeser
Copy link
Member

This should probably be a header flag in the channel frame. There's space left for a flag - this way the field is optional as well so you don't have send all the time. Any thoughts about making it a setup frame flag too so you can reject a connection that tries to do this if your don't want to support this at all?

@stevegury I think the server should either accept the request n and try to process everything, or reject the request and process nothing. I think doing anything else would make it kludgey and lead to weird bugs. Besides I think like Yuri said this seems like something the client and server would have probably agreed upon ahead of time?

I think we'll have to make the wording in the spec very clear that this is entirely up to the application, but practically it seems like a safe fallback is to buffer locally and set "reverse initial N" to 0.

I don't understand why this is necessary? If you don't send an "initial reverse N" isn't this the way that channel works today? You just buffer on back-pressure?

@lehecka
Copy link

lehecka commented Feb 20, 2018

Speaking about C++ implementation.. I can see how we can modify the RSocket library to allow emitting REQUEST_CHANNEL frame immediately followed by several PAYLOAD frames. On the responder side the application code would have to set credits (in the handleRequestChannel) to allow receiving the payloads otherwise they would be rejected and the stream closed (errored out).
So with this change we can allow the requested usecase without modifying the RS protocol spec. It would be essentially a application specific behavior.

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

No branches or pull requests

6 participants