fix: Dequeue messages when committing#4979
Closed
ingoncalves wants to merge 1 commit intoether:developfrom
Closed
Conversation
Member
|
Thanks for this! I need to do some studying to understand this change. In the meantime would you split the commit into two:
|
Member
|
Superseded by #4983. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes a fix to the issue #4978
Let's try to explain this issue.
After a thorough analysis of the code, we found that the problem occurs in the message management performed by collab_client.js. The messages handler uses a
msgQueuequeue to store all messages coming from the server. There is also a state control that admits the values IDLEandCOMMITING. The synchronization problem occurs when there is a combination of the state equal toCOMMITINGwith the non-empty queue.Every time a user makes a change to the text, a new changeset is generated, but the changes are not yet applied to the base text, being suspended until confirmation from the server. So, if the state is
IDLE(initial value), the user sends aUSER_CHANGESmessage to the server and changes its state toCOMITTING.https://github.com/ether/etherpad-lite/blob/5db0c8d1cfc3f354a5a8badce6d5c3890d428e98/src/static/js/collab_client.js#L151-L158
The server then receives and processes the message, and finally responds to the author user with a message
ACCEPT_COMMITand to the other users with the messageNEW_CHANGES. When the user receives anACCEPT_COMMITmessage, he commits the changeset to the base text and sets its status toIDLEagain.It is worth mentioning that when a new message from the server is received, the user checks the message queue. If the queue is not empty, the user queues the message and postpones its processing. The special case is when the user receives a message
NEW_CHANGES, where in addition to checking the status of the queue, he also checks if he is during a character composition. If so, the message is also queued.https://github.com/ether/etherpad-lite/blob/5db0c8d1cfc3f354a5a8badce6d5c3890d428e98/src/static/js/collab_client.js#L222
This mechanism works very quickly and the queue is almost never larger than 1. However, with a slow or problematic connection, the user may be in the
COMMITINGstate waiting for theACCEPT_COMMITfrom the server and, concurrently, he receives messagesNEW_CHANGESduring a song. In this case, the message will be queued and the status will continue asCOMMITING. When receivingACCEPT_COMMIT, the queue will not be empty and this message will also be queued, without any change of state. In this special condition,ACCEPT_COMMITwill no longer be processed and status will not be changed. Consequently, new changes will not be sent to the server, since the sending only occurs when the file isIDLE.This loop occurs in the line
102https://github.com/ether/etherpad-lite/blob/5db0c8d1cfc3f354a5a8badce6d5c3890d428e98/src/static/js/collab_client.js#L94-L105
The solution we propose is to process all messages in the queue whenever the function
handleUserChangesis called withstate == COMMITING. We add one more condition to thisif/elsestatement, after checking network issues.