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

#300 - fixing issue causing dataloss when server session is lost whil… #353

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

gentledepp
Copy link
Contributor

@gentledepp gentledepp commented Oct 13, 2020

…e client sends additional batches

1 - solves data loss issue

The issue is, that the WebServerOrchestrator just creates a new clientBatchInfo if it is null.
The issue with this is: If you send changes in batches and the server session is lost (e.g. due to an ApplicationPool recycle or because the Redis cache was cleared), it will just forget all the batches that were already sent during the ongoing session!

See the following sequencediagram for clarification:

sequenceDiagram
    participant Client
    participant Server
    loop GetLocalChanges
        Client->>Client: create 2 batch files (b0, b1, b2)
    end
    Client->>Server: Send b0
    Note right of Server: session is lost <br/>=> clientBatchInfo is null
    Client->>Server: Send b1
    Client->>Server: Send b2
    loop ApplyChanges
         Server->>Server: deserialize all batches and apply to database
         Note right of Server: Only inserts b1 and b2!<br/>b0 changes are forever lost!
    end

Loading

The only thing I had to add to fix this is a null-check:

            // ------------------------------------------------------------
            // FIRST STEP : receive client changes
            // ------------------------------------------------------------

+            // ensure that the blientBatchInfo is still available in the session - it **must** only be null when the batchIndex is 0!!
+            if (sessionCache.ClientBatchInfo == null && httpMessage.BatchIndex != 0)
+                throw new SyncException("Session loss: No batchPartInfo could found for the current sessionId. It seems the session was lost. Please try again.");

            // We are receiving changes from client
            // BatchInfo containing all BatchPartInfo objects
            // Retrieve batchinfo instance if exists
            // Get batch info from session cache if exists, otherwise create it
            if (sessionCache.ClientBatchInfo == null)
                sessionCache.ClientBatchInfo = new BatchInfo(clientWorkInMemory, Schema, this.Options.BatchDirectory);

Interestingly, your code does check whether the session is empty when the client is requesting additional batches.
So I just added a test to cover the fact that this will also not break in the future.

2 HttpMessageSendChangesRequestArgs takes a byte[] - what for?

I had to implement "OnReceiveChanges" interception
Why is the parameter HttpMessageSendChangesRequestArgs of OnSendChanges taking the binary data as a parameter?

I would argue, that when I do intercept this call, I would like to see what changes are sent.
But in the current implementation, I only do get a byte array.
The only thing I could think of was to manipulate this array to e.g. add another layer of compression/encryption. Is that the intention?
As I did not see any real sense in that, I removed this.
Instead (and also in order to create an integration test for this scenario) I changed the constructor to take a HttpMessageSendChangesRequest as a parameter instead of the byte[]:

public class HttpMessageSendChangesRequestArgs : ProgressArgs
    {
        public HttpMessageSendChangesRequestArgs(HttpMessageSendChangesRequest request) 
		{
			...
		}
	}

Likewhise, I had to add HttpMessageSendChangesRequest to the HttpMessageSendChangesRequestArgs

Additionally, the Interception happens before the changes are serialized.
That way, the interceptor can actually modify the changes sent to the server!

3 - Added integration test to ensure deferred transaction are never used by SqliteSyncProvider

Additionally, I added a specific integration test that ensures that the SqliteSyncProvider does not use deferred transactions when selecting local changes as this can also lead to data loss in the future!

…ider does **not** use deferred transactions to select local changes which could lead to data loss
@Mimetis
Copy link
Owner

Mimetis commented Oct 14, 2020

THIS. IS. AWESOME !
Thanks @gentledepp for this PR.
I'll make a quick review this week and will merge it asap !

@Mimetis Mimetis merged commit 83295ee into Mimetis:master Jan 8, 2021
@Mimetis
Copy link
Owner

Mimetis commented Jan 8, 2021

I had to implement "OnReceiveChanges" interception
Why is the parameter HttpMessageSendChangesRequestArgs of OnSendChanges taking the binary data as a parameter?
The only thing I could think of was to manipulate this array to e.g. add another layer of compression/encryption. Is that the intention?

Indeed, but since we have Converters and Serializers, I guess this interceptor is kind of obsolete now
Your implementations seems more accurate, thanks :)

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.

2 participants