Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻ Throw error for pending ops on hard rollback #626

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

dawidreedsy
Copy link
Contributor

@dawidreedsy dawidreedsy commented Oct 3, 2023

Our current flow for handling the rollback swallows some errors and discards pending ops.

  1. Whenever we do hard rollback and fetch for hard rollback fails we should throw the error so that user is informed that something really wrong happen.
  2. Whenever we preform the hard rollback and we have some pending ops and the ERR_OP_PENDING_OP_SUBMIT_REJECTED error is thrown we should soft reject, as this error is meant for the user to reject some of the ops from submition to DB, however we should throw for all the pending ops s the pending ops would be otherwise just discarded. This way we at least allow user to handle the error
  3. For the soft rollback we just resolve for this error and allow pending ops to be submitted to the server.

This PR is extracted from:

@dawidreedsy dawidreedsy marked this pull request as draft October 3, 2023 14:17
@dawidreedsy dawidreedsy force-pushed the fix-sharedb-error-submit-err branch 5 times, most recently from 066ef69 to 4ae9c50 Compare October 3, 2023 14:44
@dawidreedsy dawidreedsy changed the title CP Inform about fetchError in hard rollback Oct 3, 2023
@ericyhwang
Copy link
Contributor

From discussion between @dawidreedsy, @alecgibson, Edmond, and me:

For soft-rejecting an op:

  • The main op's callback is resolved, both in the soft rollback and hard rollback cases
  • If it was a soft rollback, then originally pending ops are re-applied onto the rolled-back version
  • If it was a hard rollback, then originally pending ops get rejected with a special hard-rollback error

That behavior should apply to edit, create, and del.

@dawidreedsy dawidreedsy force-pushed the fix-sharedb-error-submit-err branch 3 times, most recently from 92adad5 to 16ae32e Compare October 10, 2023 12:19
@dawidreedsy dawidreedsy changed the title Inform about fetchError in hard rollback ♻ Throw error for pending ops on hard rollback Oct 10, 2023
Our current flow for handling the rollback swallows some errors and discards pending ops.
1. Whenever we do hard rollback and fetch for hard rollback fails we should throw the error so that user
   is informed that something really wrong happen.
2. Whenever we preform the hard rollback and we have some pending ops
   and the `ERR_OP_PENDING_OP_SUBMIT_REJECTED` error is thrown we should
   soft reject, as this error is meant for the user to reject some of
   the ops from submition to DB, however we should throw for all the
   pending ops s the pending ops would be otherwise just discarded. This
   way we at least allow user to handle the error
3. For the soft rollback we just resolve for this error and allow
   pending ops to be submitted to the server.
@coveralls
Copy link

coveralls commented Oct 10, 2023

Coverage Status

coverage: 97.519% (+0.007%) from 97.512% when pulling a52426e on fix-sharedb-error-submit-err into 62e4ec5 on master.

@dawidreedsy dawidreedsy marked this pull request as ready for review October 10, 2023 12:23
lib/client/doc.js Outdated Show resolved Hide resolved
lib/client/doc.js Outdated Show resolved Hide resolved
lib/client/doc.js Outdated Show resolved Hide resolved
lib/client/doc.js Outdated Show resolved Hide resolved
lib/client/doc.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
test/client/presence/doc-presence.js Outdated Show resolved Hide resolved
test/client/presence/doc-presence.js Outdated Show resolved Hide resolved
test/client/submit.js Outdated Show resolved Hide resolved
var doc = connection.get('dogs', 'fido');
doc.preventCompose = true;

doc.create({age: 3});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot really as we check create without callback

lib/client/doc.js Outdated Show resolved Hide resolved
lib/client/doc.js Outdated Show resolved Hide resolved
lib/client/doc.js Outdated Show resolved Hide resolved
@dawidreedsy dawidreedsy merged commit dc033c2 into master Oct 10, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants