optimizes stream id to be non-thread-safe#812
optimizes stream id to be non-thread-safe#812OlegDokuka wants to merge 2 commits intobugfix/streamId-orderfrom
Conversation
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
| this.streamIdSupplier = | ||
| isResumeEnabled | ||
| ? new ResumableStreamIdSupplier(streamIdSupplier, receivers) | ||
| : streamIdSupplier; |
There was a problem hiding this comment.
Is checking the receivers map related to resume at all? As long as the same StreamIdSupplier is used, numbers are always going up and shouldn't be repeated.
Looking at the history, the check was introduced in e9f2efe as part of a fix for overflowing so maybe that should always be present?
There was a problem hiding this comment.
I guess this should not be the case since if we going to come to http2 transport, then once we get it overflowed - everything will be broken.
Spec also mentions that reuse of stream-id is recommended only with resumability -> https://github.com/rsocket/rsocket/blob/master/Protocol.md#lifetime.
@linux-china, @jjeffcaii do you know folks, whether it should be re-usable id all the time, I remember you were asking about that, but I'm not sure that it is always needed.
@rstoyanchev Maybe we can do a configuration?
There was a problem hiding this comment.
Alright, I guess in this PR I will just provide a relaxed stream id generation, thus going to rollback most of the changes. No need to care about all the things right now
There was a problem hiding this comment.
@OlegDokuka Yes, SteamID is re-usable by default in current Go and Rust SDK.
|
|
||
| import io.netty.util.collection.IntObjectMap; | ||
| import java.util.concurrent.atomic.AtomicLongFieldUpdater; | ||
| interface StreamIdSupplier { |
There was a problem hiding this comment.
This should have some Javadoc at least, as a reminder, that explains the class is not thread-safe and assumes the issuing of id's will not be done concurrently (e.g. serial sending of requests).
OlegDokuka
force-pushed
the
bugfix/streamId-order
branch
from
6a74390 to
7461326
Compare
OlegDokuka
force-pushed
the
bugfix/streamId-order
branch
2 times, most recently
from
dcd413b to
de75f44
Compare
4 participants
This PR is a follow up to #811 and therefore introduces a non-thread safe way to issue stream id since this class is never called in the concurrent fashion