1263339 - Use X-If-Unmodified-Since to make uploads safe

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Description

[Richard Newman [:rnewman]]

X-I-U-S is Sync's way of making sure a collection hasn't changed between the time you last saw it and the time you make a change to it. That's how we avoid missing changes or introducing inconsistencies.

There are a couple of spots in the uploader code that pass `nil`:

Sync/Synchronizers/IndependentRecordSynchronizer.swift

94-        let storageOp: ([Record<T>], Timestamp) -> DeferredTimestamp = { records, timestamp in
95:            // TODO: use I-U-S.
96-
97-            // Each time we do the storage operation, we might receive a backoff notification.
98-            // For a success response, this will be on the subsequent request, which means we don't
99-            // have to worry about handling successes and failures mixed with backoffs here.
100-            return storageClient.post(records, ifUnmodifiedSince: nil)

186-        let perChunk: ([String], Timestamp) -> DeferredTimestamp = { (lines, timestamp) in
187-            log.debug("Uploading \(lines.count) records.")
188:            // TODO: use I-U-S.
189-
190-            // Each time we do the storage operation, we might receive a backoff notification.
191-            // For a success response, this will be on the subsequent request, which means we don't
192-            // have to worry about handling successes and failures mixed with backoffs here.
193-            return storageClient.post(lines, ifUnmodifiedSince: nil)


This bug is to chain through timestamps in a way that correctly detects races.

Note that this work intersects with Bug 1253112 -- when we do atomic uploads we'll do them to a dedicated batch and pass in the timestamp in one go. If that work happens first, morph or dupe this bug.

Comment 1

[Richard Newman [:rnewman]]

There's also a bug in StorageClient:

    public func post(lines: [String], ifUnmodifiedSince: Timestamp?) -> Deferred<Maybe<StorageResponse<POSTResult>>> {
        let deferred = Deferred<Maybe<StorageResponse<POSTResult>>>(defaultQueue: client.resultQueue)

        if self.client.checkBackoff(deferred) {
            return deferred
        }

        let req = client.requestPOST(self.collectionURI, body: lines, ifUnmodifiedSince: nil)


That last 'nil' should be 'ifUnmodifiedSince'.

Comment 2

[:Grisha Kruglov]

Did you end up fixing this as part of Bug 1253112?

Comment 3

[Stephan Leroux [:sleroux]]

Looks like it does since we always pass in the lastModified timestamp into the batching client if it's not 0. I'll go ahead and mark this as RESOLVED.