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

Requests may have non-sequential stream ids #749

Closed
mostroverkhov opened this issue Mar 2, 2020 · 7 comments · Fixed by #811
Closed

Requests may have non-sequential stream ids #749

mostroverkhov opened this issue Mar 2, 2020 · 7 comments · Fixed by #811
Labels
bug superseded Issue is superseded by another

Comments

@mostroverkhov
Copy link
Member

mostroverkhov commented Mar 2, 2020

Happens when request (for all interactions) subscriptions race

RequestResponseFrameFlyweight.encode(
- so frames are sent not in same order as stream ids are created
int streamId = streamIdSupplier.nextStreamId(receivers);
. 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
Copy link
Member

OlegDokuka commented Mar 2, 2020

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
Copy link
Member Author

mostroverkhov commented Mar 3, 2020

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
Copy link
Member

OlegDokuka commented Mar 3, 2020

@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
Copy link
Member Author

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
Copy link
Member

OlegDokuka commented May 15, 2020

@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
Copy link
Member Author

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
Copy link
Member

OlegDokuka commented May 15, 2020

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

@OlegDokuka OlegDokuka removed this from the 1.0 milestone Jun 2, 2020
@OlegDokuka OlegDokuka added the superseded Issue is superseded by another label Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug superseded Issue is superseded by another
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants