collab_client: Fix race condition with character composition

[rhansen]

Multiple commits:

• collab_client: Factor out duplicate ACCEPT_COMMIT code
• collab_client: Delete unused caughtErrors
• collab_client: Convert state var to committing bool
• collab_client: Use Date.now() instead of casting a Date object
• collab_client: Redo server message queueing
• collab_client: Delete unused NO_COMMIT_PENDING handling
• tests: Add regression tests for character composition race

This should fix #4978. Also see PR #4979, which is a different approach to fix the same problem.

@ingoncalves What are your thoughts about this approach?

[ingoncalves]

Thanks a lot for this fix proposal!

I checked out this branch and ran the test that covers this bug and everything looks fine.
My first suggestion is to add the helper src/tests/frontend/helper/multipleUsers.js and the test src/tests/frontend/specs/collab_client.js from #4979 to this PR in order to cover this fix and ensure that it will not break again in the future.

My second suggestion is in relation to serverMessageTaskQueue. As I am not used to this code and as it has a complex logic, it was a little complicated to understand what this code does. So, I commented on some suggestions that can help other people to understand it better.

My last suggestion is not specifically about this PR. We could be able to have unit tests in the front-end as well. Today, we only have integrations tests, and some components, such as collab_client.js, contain a complex logic that could be better stressed within unit tests. It would also allow simulating complex scenarios, such as network problems. If you agree, we could manage to set up the tools necessary to have unit tests in the front-end in the future.

[rhansen]

... add the helper src/tests/frontend/helper/multipleUsers.js and the test src/tests/frontend/specs/collab_client.js from #4979 to this PR in order to cover this fix and ensure that it will not break again in the future.

Will do.

My last suggestion is not specifically about this PR. We could be able to have unit tests in the front-end as well. Today, we only have integrations tests, and some components, such as collab_client.js, contain a complex logic that could be better stressed within unit tests. It would also allow simulating complex scenarios, such as network problems. If you agree, we could manage to set up the tools necessary to have unit tests in the front-end in the future.

I would love to have better unit tests. Unfortunately, tests take time to write, and unless people encounter more bugs with this code I think that adding more tests is low priority. I will happily accept PRs that add tests, but I don't have the time to write them myself.

[ingoncalves]

I would love to have better unit tests. Unfortunately, tests take time to write, and unless people encounter more bugs with this code I think that adding more tests is low priority. I will happily accept PRs that add tests, but I don't have the time to write them myself.

Great!!! So we will manage to add some unit tests soon. At least, it will be a starting point!

[rhansen]

I pushed some revisions and tests.

@ingoncalves: Would you review the commits starting with "tests: Add regression tests for character composition race"? If they look OK to you I'll squash the "for squash" commits into the "tests: Add regression tests for character composition race" commit (so you'll be the one with the credit/blame) unless you want those changes to be separate.

[webzwo0i]

With the old behavior the only place where msgQueue is filled for the first(!) time is during processing of a NEW_CHANGES message. On ACCEPT_COMMIT or CLIENT_RECONNECT the msgQueue was not used even when composing, unless a NEW_CHANGES was received before and the client was already composing. The new behavior is to wait for compositionend no matter which of the above three message types is received, right?

I don't think it matters, as long as order of message processing is retained.

[ingoncalves]

I pushed some revisions and tests.

@ingoncalves: Would you review the commits starting with "tests: Add regression tests for character composition race"? If they look OK to you I'll squash the "for squash" commits into the "tests: Add regression tests for character composition race" commit (so you'll be the one with the credit/blame) unless you want those changes to be separate.

Nice work! LGTM!

[rhansen]

The new behavior is to wait for compositionend no matter which of the above three message types is received, right?

Yes, nice catch.

I don't think it matters, as long as order of message processing is retained.

It might matter for timeouts (if the user is taking a long time to compose). I'll change it to match the previous behavior.

[rhansen]

I'll change it to match the previous behavior.

Done. We might also want to defer CLIENT_RECONNECT until composition completes, but at least this PR doesn't change the existing behavior (other than fix the race).