possible fix for issues with rebased number

johanneswilm commented

2018-03-11 12:54:09 +01:00

(Migrated from github.com)

As part of automatic testing, we have one test in which two users in two browsers try to write in the same document simultaneously and one of them tries to use the undo function. This threw several errors every now and then. Looking at the code here and in the history module [1], it seems that this rebase count it comes up with her is the number of unconfirmed steps it tries to rebase, whereas in the history module it relies on the actual number of steps that have been rebased. The number will be the same as long as all steps are successfully rebased, but the count is off if maybeStep fails. By returning the actual number of unconfirmed steps here, the error did not throw any more in a number of test runs.

[1] github.com/ProseMirror/prosemirror-history@528b1e9fa4/src/history.js (L140)

As part of automatic testing, we have one test in which two users in two browsers try to write in the same document simultaneously and one of them tries to use the undo function. This threw several errors every now and then. Looking at the code here and in the history module [1], it seems that this rebase count it comes up with her is the number of unconfirmed steps it tries to rebase, whereas in the history module it relies on the actual number of steps that have been rebased. The number will be the same as long as all steps are successfully rebased, but the count is off if maybeStep fails. By returning the actual number of unconfirmed steps here, the error did not throw any more in a number of test runs. [1] https://github.com/ProseMirror/prosemirror-history/blob/528b1e9fa4f393b4140c4ea67874625dedede8e1/src/history.js#L140

marijnh commented

2018-03-12 10:28:25 +01:00

(Migrated from github.com)

That does look good. Do you have a specific (dependency-less) test case that we can add for this problem?

johanneswilm commented

2018-03-12 10:49:53 +01:00

(Migrated from github.com)

Not dependencyless, no. In our tests we are running integration tests with two browsers with a ton of dependencies/infrastructure. It's also not the best way to check whether this issue is still there as the bug only showed up every now and then.

I guess you'd need as a minimum a collaborative editor setup with a basic document with unconfirmed steps that you know will fail if rebased on top of another series of steps.

Not dependencyless, no. In our tests we are running integration tests with two browsers with a ton of dependencies/infrastructure. It's also not the best way to check whether this issue is still there as the bug only showed up every now and then. I guess you'd need as a minimum a collaborative editor setup with a basic document with unconfirmed steps that you know will fail if rebased on top of another series of steps.

johanneswilm commented

2018-03-12 10:58:12 +01:00

(Migrated from github.com)

Looking at the test files in prosemirror-collab and prosemirror-history... I guess you'd need a setup with both of those modules loaded as you need both rebased steps and the do undo. If you want me to, I can try to get something going, but most likely you will know better what will need to be done to create such tests and where they should live.

marijnh commented

2018-03-13 17:40:59 +01:00

(Migrated from github.com)

The bug was actually in the history plugin—the count noted in the transaction does refer to the count of the old steps that were rebased. github.com/ProseMirror/prosemirror-history@2c41d30876 should fix this. I've released it as prosemirror-history 1.0.2

The bug was actually in the history plugin—the count noted in the transaction does refer to the count of the _old_ steps that were rebased. https://github.com/ProseMirror/prosemirror-history/commit/2c41d308769cca5158359c96c83b6a14e5f2e7d3 should fix this. I've released it as prosemirror-history 1.0.2

johanneswilm commented

2018-03-13 19:14:00 +01:00

(Migrated from github.com)

Ok, I guess it can be made to have either meaning.

I still get a RangeError, but only about 50% as much as earlier. So this may be unrelated. Sorry that I don't have better data.

Just for understanding: The rebased transaction consists of three groups of steps in this order:

The reversed unconfirmed steps. The number of these steps is certain and corresponds to the rebased number
The incoming steps. This number is also certain.
The rebased unconfirmed steps. The number of these steps is as high as the steps in 1 or lower.

I was under the impression that the rebased number referred to the number used in 3, and that start was the index number of the first rebased step. But that doesn't seem to be the case now.

So if I need to get the latest confirmed doc, instead of going backward through steps of type 3, I need to go through the steps of type 1 and 2, as I described it in the forum [1], right?

[1] https://discuss.prosemirror.net/t/conversion-of-variables-and-functions-0-10-0-12/459/34?u=johanneswilm

Ok, I guess it can be made to have either meaning. I still get a RangeError, but only about 50% as much as earlier. So this may be unrelated. Sorry that I don't have better data. Just for understanding: The rebased transaction consists of three groups of steps in this order: 1. The reversed unconfirmed steps. The number of these steps is certain and corresponds to the rebased number 2. The incoming steps. This number is also certain. 3. The rebased unconfirmed steps. The number of these steps is as high as the steps in 1 or lower. I was under the impression that the rebased number referred to the number used in 3, and that `start` was the index number of the first rebased step. But that doesn't seem to be the case now. So if I need to get the latest confirmed doc, instead of going backward through steps of type 3, I need to go through the steps of type 1 and 2, as I described it in the forum [1], right? [1] https://discuss.prosemirror.net/t/conversion-of-variables-and-functions-0-10-0-12/459/34?u=johanneswilm

johanneswilm commented

2018-03-13 19:31:13 +01:00

(Migrated from github.com)

The traceback for the error that is still thrown looks like this:

details: http://localhost:8081/static/js/transpile/editor.js?v=1520963571: 83738: Uncaught RangeError: Position 34 out of range, 55, RangeError: Position 34 out of range
Function.resolve (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:83738:55)
at Function.resolveCached (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:83761:60)
at Node.resolve (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:84031:70)
at TextBookmark.resolve (<anonymous>:4213:62)
at histTransaction (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:81737:33)
at undo (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:81819:19)

I looked up what the histTransaction line was in the original sources, and corresponds to this one:
https://github.com/ProseMirror/prosemirror-history/blob/master/src/history.js#L317

The traceback for the error that is still thrown looks like this: details: http://localhost:8081/static/js/transpile/editor.js?v=1520963571: 83738: Uncaught RangeError: Position 34 out of range, 55, RangeError: Position 34 out of range Function.resolve (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:83738:55) at Function.resolveCached (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:83761:60) at Node.resolve (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:84031:70) at TextBookmark.resolve (<anonymous>:4213:62) at histTransaction (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:81737:33) at undo (http://localhost:8081/static/js/transpile/editor.js?v=1520963571:81819:19) I looked up what the histTransaction line was in the original sources, and corresponds to this one: https://github.com/ProseMirror/prosemirror-history/blob/master/src/history.js#L317

marijnh commented

2018-03-14 10:27:23 +01:00

(Migrated from github.com)

I was under the impression that the rebased number referred to the number used in 3

It definitely refers to 1.

So if I need to get the latest confirmed doc, instead of going backward through steps of type 3, I need to go through the steps of type 1 and 2, as I described it in the forum [1], right?

That should work, yes.

The other problem seems to be a different issue—if you can isolate a scenario that reproduces it I'd be interested.

> I was under the impression that the rebased number referred to the number used in 3 It definitely refers to 1. > So if I need to get the latest confirmed doc, instead of going backward through steps of type 3, I need to go through the steps of type 1 and 2, as I described it in the forum [1], right? That should work, yes. The other problem seems to be a different issue—if you can isolate a scenario that reproduces it I'd be interested.

johanneswilm commented

2018-03-14 23:06:05 +01:00

(Migrated from github.com)

The other problem seems to be a different issue—if you can isolate a scenario that reproduces it I'd be interested.

Ok, if I discover a way to isolate it, I'll let you know.

> The other problem seems to be a different issue—if you can isolate a scenario that reproduces it I'd be interested. Ok, if I discover a way to isolate it, I'll let you know.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

possible fix for issues with rebased number #1

Pull request closed