rsocket / rsocket-java

Java implementation of RSocket
http://rsocket.io
Apache License 2.0
2.36k stars 354 forks source link

Requests may have non-sequential stream ids #749

Closed mostroverkhov closed 4 years ago

mostroverkhov commented 4 years ago

Happens when request (for all interactions) subscriptions race https://github.com/rsocket/rsocket-java/blob/c66dfb8b6d9d1a51655cde43dcf56666fa251162/rsocket-core/src/main/java/io/rsocket/RSocketRequester.java#L218 - so frames are sent not in same order as stream ids are created https://github.com/rsocket/rsocket-java/blob/c66dfb8b6d9d1a51655cde43dcf56666fa251162/rsocket-core/src/main/java/io/rsocket/RSocketRequester.java#L209. Other than being against spec this makes implementing http2 streams transport harder that necessary - http2 request stream ids are strictly sequential also as I cant trust rsocket stream ids. Just for discussion - will have reproducer by the end of the week.

OlegDokuka commented 4 years ago

Other than being against spec this makes implementing http2 streams

Can you please point to a certain spec section that says anything on that?

request stream ids are strictly sequential also as I cant trust rsocket stream ids. Just for discussion - will have reproducer by the end of the week.

I can not see any possible solution without involving complex synchronization in order to ensure the streams ID are identical to the order in which payloads come into the network. Even though the id is generated at the same time the payload, there is always racing on delivering a frame.

Also, it makes sense for HTTP/2 (https://tools.ietf.org/html/rfc7540#section-5.1.1) which is based on TCP connection but now imagine Aeron or QUIC connection which does not guarantee anything in terms of the order of frames so having such a restriction just going to complicate implementation unreasonably.

I guess the reference to HTTP/2 is more on the fashion how ids are generated rather than on that particular restriction related to the fact that HTTP/2 is TCP based

What do you think on that @nebhale @smaldini @rstoyanchev @rdegnan @linux-china @stevegury @yschimke?

mostroverkhov commented 4 years ago

Can you please point to a certain spec section that says anything on that?

I read that as they should be sequential

Stream IDs on the client MUST start at 1 and increment by 2 sequentially, such as 1, 3, 5, 7, etc.
Stream IDs on the server MUST start at 2 and increment by 2 sequentially, such as 2, 4, 6, 8, etc.

which is based on TCP connection but now imagine Aeron or QUIC connection which does not guarantee anything in terms of the order of frames

Transports are required to send all frames in order. Given above, if there are 2 client requests, there should be two frames on wire with ids 1,3, both sent and received in that order.

I can not see any possible solution without involving complex synchronization

One solution from top of my head - existing (and future ones, like quic) transport implementations are netty based so there is EventLoop that can be used as scheduler for serialization. This should have negligible performance impact - done by (reactor)netty anyways, just pull this task up from transport to rsocket.

OlegDokuka commented 4 years ago

@mostroverkhov the implementation does not contradict anything said in the spec. StreamIdSupplier issues ids sequentially.

One solution from top of my head - existing (and future ones, like quic) transport implementations are netty based so there is EventLoop that can be used as scheduler for serialization. This should have negligible performance impact - done by (reactor)netty anyways, just pull this task up from transport to rsocket.

yeah, that works out


A QUIC stream provides reliable in-order delivery of bytes, but makes no guarantees about order of delivery with regard to bytes on other streams.

Due to HTTP/3 spec, messages within a logical stream are delivered in order but there are no guarantees of delivering messages in total order see (https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-6).

It means that issuing stream id 1, flushing it to the network, issuing stream id 3 flushing it to the network may end up delivering stream id 3 earlier than stream id 1, which is not a big deal. I guess the most important is that packets within a logical stream are delivered with the order of packets issuing.

Maybe we have to clarify spec with regards to upcoming network protocols.

What do you think @nebhale @linux-china @smaldini @rdegnan @yschimke @tmontgomery @rstoyanchev @spencergibb

mostroverkhov commented 4 years ago

This looks like a good start - can you explain why introduce separate scheduler instead of delegating to transport which likely has event loop already @OlegDokuka @rstoyanchev ?

OlegDokuka commented 4 years ago

@mostroverkhov Having a scheduler in on the requester side is a temporary fix as the result of improper architecture.

  1. We do not want to introduce new API which one has to support (e.g new method alloc on the duplex connection)
  2. Executing requests on the event loop may impact generally impact (client / server) throughput

Thus, the simples and the most straight forward solution for achieving serial ID issuing to satisfy possible implementation of HTTP/2 based transport was using a separate ThredPool (e.g Schedulers.single(Schedulers.parallel()))

mostroverkhov commented 4 years ago

The thing is request frames end up on transport event loop anyways, you just add redundant scheduler queue without possibility to opt out - not everyone needs parallel scheduler, but many would benefit if transport scheduler was exposed on RSocket interface so clients can use them in respective operators without hopping threads

OlegDokuka commented 4 years ago

As I mentioned before, the need to use eventLoop is specifically to support Http/2 case. It Introducing a new API that does not pay the bill for future support, especially taking into account that this code will be eliminated and redesigned. Thus, RSocketRequester will be unaware of streamId at all and its issuing will be a transport-specific concern.

Therefore, having that minimal theoretical overhead is not a big deal