744187 - Use X-Last-Modified for conflict detection

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

Attachment: docs patch to add X-Last-Modified header

(this bug results from discussion on services-dev, which can be found here: https://mail.mozilla.org/private/services-dev/2012-April/001239.html)

This patch adds a new "X-Last-Modified" header to the SyncStorage protocol.  It is to the standard Last-Modified header what X-If-Unmodified-Since is to the standard If-Unmodified-Since header.

The intended use is for coordination and conflict-detection, and I've added some basic examples in the doc to show how it should be used in conjunction with existing timestamp-based functionality.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: patch adding X-Last-Modifed header to syncstorage server

This patch makes SyncStorage (and, transitively, AITC) output the X-Last-Modified header where appropriate.  It doesn't enforce the consistency semantics that are described in the docs patch, but at least it will allow us to enable them with a purely server-side change.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 613769 [details] [diff] [review]
docs patch to add X-Last-Modified header

Review of attachment 613769 [details] [diff] [review]:
-----------------------------------------------------------------

::: source/storage/apis-2.0.rst
@@ +76,5 @@
>  
> +Each collection has a last-modified timestamp corresponding to the time of
> +the last BSO addition, deletion or modification in the collection.  This
> +timestamp can be used for coordination and conflict-detection as described
> +in :ref:`syncstorage_concurrency`.

Add a note about behavior after deleting the last BSO from a collection.

@@ +482,5 @@
> +an atomic unit:
> +
> +  * Take the current timestamp on the server; call this timestamp `Tw`.
> +  * Check that `Tw` is less than or equal to the last-modified time of the
> +    target collection; if not then a **409 Conflict** response is generated.

Can this ever happen? Clock drift between webheads?

@@ +483,5 @@
> +
> +  * Take the current timestamp on the server; call this timestamp `Tw`.
> +  * Check that `Tw` is less than or equal to the last-modified time of the
> +    target collection; if not then a **409 Conflict** response is generated.
> +  * Create any news BSOs as specified by the request, setting

s/news/new

@@ +490,5 @@
> +    their last-modified timestamp to `Tw`.
> +  * Delete any BSOs as specified by the request.
> +  * Set the last-modified time of the collection to `Tw`.
> +  * Generate a **201** or **204** response with the **X-Last-Modified** and
> +    **X-Timestamp** headers set to `Tw`.

So for a write these are always guaranteed to be equal?

@@ +544,5 @@
> +Example: safely updating items in a collection
> +----------------------------------------------
> +
> +To update items in a collection without overwriting any changes made by other
> +clients, use **POST /storage/<collection>** with the **X-If-Unmodified-Since**

... or PUT.

@@ +674,4 @@
>      * "Weave Basic Objects" have been renamed "Basic Storage Objects".
>      * The "Weave" prefix has been removed from all custom headers.
>  
> +* Authentication is now performed using the Sagrada ServerServer flow and

ServerServer?

@@ +679,2 @@
>  
> +* The structure of the endpoint url is no longer specified, and should be

s/url/URL

Comment 3

[Ryan Kelly [:rfkelly]]

Header added in SyncStorage and AITC:

https://github.com/mozilla-services/server-syncstorage/commit/7701efa98008ef0d92f311d9890b273f89ecdb69
https://github.com/mozilla-services/server-aitc/commit/6b2e2120274a7c5d70969382de2c57df0fa96767

Leaving the bug open to keep working on the docs.

Comment 4

[Ryan Kelly [:rfkelly]]

(In reply to Richard Newman [:rnewman] from comment #2)
> Comment on attachment 613769 [details] [diff] [review]
> docs patch to add X-Last-Modified header
> 
> Review of attachment 613769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: source/storage/apis-2.0.rst
> @@ +76,5 @@
> >  
> > +Each collection has a last-modified timestamp corresponding to the time of
> > +the last BSO addition, deletion or modification in the collection.  This
> > +timestamp can be used for coordination and conflict-detection as described
> > +in :ref:`syncstorage_concurrency`.
> 
> Add a note about behavior after deleting the last BSO from a collection.

I'm not sure what to say about this - deleting the last BSO from a collection will bump the collection modified-time the same as any other deletion, it's not a special case in any way.

(this isn't quite right in the current server code, but that's a bug in the server code not in the spec)

Comment 5

[Toby Elliott [:telliott]]

You could note that deleting all items from a collection will not generate a 404; it requires actual deleting of the collection.

Not strictly necessary, but wouldn't hurt to be clear in an area that isn't necessarily intuitive.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Toby Elliott [:telliott] from comment #5)
> You could note that deleting all items from a collection will not generate a
> 404; it requires actual deleting of the collection.
> 
> Not strictly necessary, but wouldn't hurt to be clear in an area that isn't
> necessarily intuitive.

Yup, that was my point -- the purpose of a spec is to communicate an understanding to the reader, and this is probably the right place to point out that empty collections exist, and collections have a lifetime longer than their contained objects. For example:


+Each collection has a last-modified timestamp corresponding to the time of
+the last BSO addition, deletion or modification in the collection. An empty
+collection's last-modified timestamp is the time of the deletion of its last
+contained object.
+
+This timestamp can be used for coordination and conflict-detection as described
+in :ref:`syncstorage_concurrency`.
+
+To completely delete the collection, a client must issue a `DELETE` request
+for the collection itself, or for the entire /storage/ URL space.

Comment 7

[Ryan Kelly [:rfkelly]]

Delete clarifications done over on Bug 746040.

Comment 8

[Ryan Kelly [:rfkelly]]

Docs patch committed here: https://github.com/mozilla-services/docs/commit/322738104220f33cde2c4bdf4a3b428b1d06a562

I'm a little concerned that the consistency/atomicity guarantees I've put in there are too strong - they will require us to run our database transactions at SERIALIZABLE isolation, and might make it harder to implement a non-RDBMS backend in the future.  On the other hand, they're easy to explain and to reason about, and they don't leave any subtle edge-cases that might catch the client by surprise.  So I'm happy with that for now.

Comment 9

[James Bonacci [:jbonacci]]

QA can verify this in docs then close.

Comment 10

[James Bonacci [:jbonacci]]

OK.
Code verified and passed functional tests of Sync 2.0 and AITC 1.0 for a 4/23/2012 deploy to qa1.
Also verified here: 
http://docs.services.mozilla.com/storage/apis-2.0.html#concurrency-and-conflict-management