fix(server): message handling order when using beforeHandleMessage (#880)
#901
Conversation
|
|
||
| message.writeVarString(documentName) | ||
|
|
||
| this.callbacks.beforeHandleMessage(this, data) | ||
| .then(() => { | ||
| this.nextProcessBeforeHandleMessage() |
There was a problem hiding this comment.
The ordering of things here seems a bit off to me: you trigger the processing of a next message before you've processed this one (below).
There was a problem hiding this comment.
The invocation of this.nextProcessBeforeHandleMessage will initiate a new async sequence and to me it seems possible that things go out-of-order again.
| @@ -41,6 +41,10 @@ export class Connection { | |||
|
|
|||
| logger: Debugger | |||
|
|
|||
| private beforeHandleMessageQueue: Uint8Array[] = [] | |||
There was a problem hiding this comment.
You might be able to do with just 1 additional state variable if you shifted the message only after successfully processing it.
| } | ||
|
|
||
| /** Process an incoming message */ | ||
| private processBeforeHandleMessage(data: Uint8Array): void { |
There was a problem hiding this comment.
The name of the method is out of sync with the comment. I like the comment more, as what this methos does is process an incoming message after applying the beforeHandleMessage hooks.
| } | ||
|
|
||
| /** Process an incoming message */ | ||
| private processBeforeHandleMessage(data: Uint8Array): void { |
There was a problem hiding this comment.
I would prefer using async-await syntax for this method. This would allow you to use a while (this. beforeHandleMessageQueue.length > 0 loop.
No description provided.