Skip to content

Conversation

@beaucollins
Copy link
Contributor

@beaucollins beaucollins commented Sep 23, 2019

Unit test to simulate a client receiving a 409 error from the server for a change that originated from the same client.

The fix is intentionally left out which should be as simple as:

diff --git a/src/simperium/channel.js b/src/simperium/channel.js
index 4f59d15..15db61b 100644
--- a/src/simperium/channel.js
+++ b/src/simperium/channel.js
@@ -238,6 +238,7 @@ internal.handleChangeError = function( err, change, acknowledged ) {
 			}
 
 			break;
+		case 409:
 		case CODE_EMPTY_RESPONSE: // Change causes no change, just acknowledge it
 			internal.updateAcknowledged.call( this, acknowledged );
 			break;

@beaucollins beaucollins changed the title Add/test 409 error Unit Test: 409 server error Sep 23, 2019
var data = { title: 'hola mundo' };

channel.on( 'acknowledge', function() {
channel.once( 'acknowledge', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, any future channel.emit('acknowledge') causes a test to error because done is called more than once.

/**
* Simulate receiving a 409
*/
channel.handleMessage( 'c:[{"error": 409, "ccids":["dup-ccid"], "id": "mock-id"}]' );
Copy link
Contributor Author

@beaucollins beaucollins Sep 23, 2019

Choose a reason for hiding this comment

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

See: https://github.com/Simperium/simperium-protocol/blob/master/SYNCING.md#receiving-remote-changes

c:[{"clientid":"offending-client","id":"object-key","error":"401","ccids":["abcdef123456"]}]

@beaucollins
Copy link
Contributor Author

Worth noting in the protocol docs it says:

409 : Duplicate change, client should stop sending this change and retrieve changes since its last syncd cv to process the changes list from the server. In normal case the remote changes list will contain the change that caused the duplicate error. If it doesn't, client will need to re-load this object. In either case, client will know that the server version of the object already incorporates the delta that this change contains.


break;
case CODE_DUPLICATE_CHANGE:
internal.updateAcknowledged.call( this, acknowledged );
Copy link
Contributor

Choose a reason for hiding this comment

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

I left this as a separate case since it carries a different semantic meaning than an empty change

@dmsnell
Copy link
Contributor

dmsnell commented Sep 23, 2019

Let's :shipit: and continue tracking the errors. This doesn't solve the problem I found in Automattic/simplenote-electron#1579 but it does seem to be a hidden bug.

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.

3 participants