Java client connection state part 1 #24166
Conversation
| } else { | ||
| negotiate = tokenCompletable.andThen(Single.defer(() -> Single.just(new NegotiateResponse(baseUrl)))); | ||
| } | ||
| hubConnectionState = HubConnectionState.CONNECTING; |
There was a problem hiding this comment.
We should do verbose logging for each state transition.
| // subscribe makes this a "hot" completable so this runs immediately | ||
| }).subscribeWith(start); | ||
| // subscribe makes this a "hot" completable so this runs immediately | ||
| }).subscribe(() -> { |
There was a problem hiding this comment.
The CONNECTING->CONNECTED state transition is hidden in this diff, but we should assert the previous state where we can.
There was a problem hiding this comment.
I have a PR that extracts this into a method, I'll add the logging though
| // On start we're going to receive the handshake response and also an invocation in the same payload. | ||
| hubConnection.start(); | ||
| mockTransport.getStartTask().timeout(1, TimeUnit.SECONDS).blockingAwait(); | ||
| String sentMessage = handshakeMessageTask.timeout(1, TimeUnit.SECONDS).blockingGet(); |
There was a problem hiding this comment.
Do these short 1 second timeouts ever cause flakiness? If not, I'm impressed.
There was a problem hiding this comment.
There were one or two flaky tests in java but I don't think they were ever related to these timeouts.
And actually, yesterday I was looking into them and there is a pretty good reason these never trigger. We can talk about it later today.
src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java
Outdated
Show resolved
Hide resolved
src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java
Outdated
Show resolved
Hide resolved
BrennanConroy
force-pushed
the
brecon/state
branch
from
501a28a to
d551a9a
Compare
| hubConnectionStateLock.lock(); | ||
| try { | ||
| if (hubConnectionState != HubConnectionState.DISCONNECTED) { | ||
| logger.debug("Another start is in progress. Waiting for start to complete."); |
There was a problem hiding this comment.
The hub could be already-started here too, right? Maybe log the current state.
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
I don't love approving this without the changes to assert previous states and log state transitions, but I trust those are coming.
BrennanConroy
added
the
tell-mode
I'm limiting this change to just adding the
CONNECTINGstate and modifying a couple things so it works as expected.Part 2 will fix things like calling stop while in the middle of start, but I want to clean up the code a bit before doing that, so I'm planning on making a "clean up" PR before doing Part 2.
Easier to review if you turn off whitespace changes.
Progress towards #23291