Add delay helper to fix session not found
#21
Conversation
nerzhulart
force-pushed
the
n500/delay-incoming-messages
branch
from
8ecfc3d to
90f5a44
Compare
| pendingIncomingRequests.update { map -> map.put(requestId, job) } | ||
| }.invokeOnCompletion { | ||
| pendingIncomingRequests.update { it.remove(requestId) } | ||
| messageProcessingMutex.withLock { |
There was a problem hiding this comment.
I see a potential deadlock here.
Let's imagine that we have one established session, then, the following steps will block us completely:
- we are starting another one in
newSession - we have sent the
session/newrequest - before the agent answers to it, it sending us
sessionUpdatefor already established session - after that agent send us the
NewSessionResponse
As you can see, on step 1 we locked this mutex, and on step 3 we suspended inside the handleIncomingMessage, so we will never process the message from step 4
There was a problem hiding this comment.
Ok! I reworked it using your approach with a counter
erokhins
left a comment
erokhins
left a comment
There was a problem hiding this comment.
See comment about deadlock :(
nerzhulart
force-pushed
the
n500/delay-incoming-messages
branch
from
90f5a44 to
d3c47af
Compare
erokhins
left a comment
erokhins
left a comment
There was a problem hiding this comment.
Personal preference: I would remove the sessionDeferred part.
Still, approved even in the current way -- wasn't able to break it.
P.s. probably it deserves a test -- it could be implemented by detay in the factory.createClientOperations(sessionId, sessionResponse)
|
|
||
| private suspend fun getSessionOrThrow(sessionId: SessionId): ClientSessionImpl = (_sessions.value[sessionId] ?: acpFail("Session $sessionId not found")).await() | ||
| private suspend fun getSessionOrThrow(sessionId: SessionId): ClientSessionImpl { | ||
| _sessions.value[sessionId]?.let { |
There was a problem hiding this comment.
This part is somewhat redundant, i.e. the whole completableDeferred could be removed and replaced with just a map to the session. In case of only one session initialization the behavious will be exactly the same, because the _currentlyInitializingSessionsCount will become 0 right after the sessionDeferred.complete(session).
And case of simultanious session initialization seems to be rare (i.e. we shouldn't optimize for it)
#3