739845 - Allow SyncStorage write requests to return "409 Conflict"

Status: VERIFIED FIXED

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

Description

[Ryan Kelly [:rfkelly]]

This is the first step towards various levels of "session locking" in syncstorage.

Write requests are allowed to return a "409 Conflict" response to indicate that some other client is editing that resource, and hence the write request has been blocked to avoid data loss/overwriting.  This response can optionally include a Retry-After head to indicate the expected completion time of the conflicting edit.

The conditions that determine whether a 409 is returned are deliberately unspecified as they may change as things evolve.  Initially it will be based on server-side locking as described in Bug 735085.  Eventually it might be based on explicit client-driven session locking, which can be done as a backwards-compatible extension to the API.

Thoughts?

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: patch to add 409-Conflict to allowed status codes

Oh, apparently I forgot to attach the patch.  Here 'tis.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 610759 [details] [diff] [review]
patch to add 409-Conflict to allowed status codes

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

I like the direction. However, I'd really like to see a comprehensive proposal on how locking will be implemented before I sign off on anything, so I'm cancelling the review.

I /think/ your usage of 409 and Retry-After is in conformance with the spec. While https://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-19#section-10.8 doesn't mention that Retry-After can be used by 409, it doesn't forbid it. The semantics definitely line up. The only thing you may want to add is a body on 409's that gives more detail. For that, we may have to wait until a more comprehensive locking proposal is in place so we know what we can put there.

You may consider cherry-picking locking features from RFC 4918 (WebDAV). Specifically 423 Locked. Although, it SHOULD carry with it some other WebDAV baggage, which might be too heavyweight for our needs. Tread with caution.

Comment 3

[Ryan Kelly [:rfkelly]]

> I like the direction. However, I'd really like to see a comprehensive proposal on how locking
> will be implemented before I sign off on anything, so I'm cancelling the review.

Fair enough.  However, note that this is not *just* for use with client-driven locking.  It's also to give the server a chance to bail on conflicting write requests like the one behind Bug 691315.

It's not the only option available for these cases of course - the server could do its own locking, blocking and retry logic to resolve conflicting writes without informing the client.  But returning a 409 seems (a) easier and (b) less likely to result in data corruption if I mess up the logic.

Maybe it's a mistake to conflate this with client-driven locking at this stage.  For example, neither Bug 691315 or Bug 735085 could sensibly generate a Retry-After header.

> I /think/ your usage of 409 and Retry-After is in conformance with the spec.

Heh, I deliberated long and hard about whether that was acceptable and didn't come up with a definitive answer.  I felt a lot better after learning that it was allowed on both 503 and 3XX class responses, rather than just 503.

Comment 4

[Gregory Szorc [:gps]]

(In reply to Ryan Kelly [:rfkelly] from comment #3)
> > I like the direction. However, I'd really like to see a comprehensive proposal on how locking
> > will be implemented before I sign off on anything, so I'm cancelling the review.
> 
> Fair enough.  However, note that this is not *just* for use with
> client-driven locking.  It's also to give the server a chance to bail on
> conflicting write requests like the one behind Bug 691315.

In that case, r+. (I'd update Bugzilla if it wouldn't take 2 changes.)

Comment 5

[Ryan Kelly [:rfkelly]]

Is it worth keeping Retry-After in the current iteration, or should we call YAGNI and punt until it's clearly needed as part of client-driven-locking in Sync2.1?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Ryan Kelly [:rfkelly] from comment #5)
> Is it worth keeping Retry-After in the current iteration, or should we call
> YAGNI and punt until it's clearly needed as part of client-driven-locking in
> Sync2.1?

The earlier it's in the spec, the earlier it can be implemented by clients.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 610759 [details] [diff] [review]
patch to add 409-Conflict to allowed status codes

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

::: source/storage/apis-2.0.rst
@@ +307,4 @@
>  
>      Possible HTTP error responses:
>  
> +    - **409 Conflict:**  another client is making changes that may conflict

… or has made, right? 409 is defined as there being a conflict with the current state of the resource, so I'd be inclined to keep our definition as broad and compliant as we can.

Note that 409 is documented as being retryable, so we will include in the body enough information to know when and how.

@@ +404,5 @@
>      further requests to the server for the number of seconds specified in
>      the header value.
>  
> +    When sent with a HTTP 409 status code, this header gives the time after
> +    which the conflicting operation is expected to be complete.

Will it always be included with a 409 (even if just an estimate), or is it optional? What should clients do if it's not?

@@ +499,5 @@
>  
>  
> +**409 Conflict**
> +
> +    Another client is currently making changes to the resource targetted by

Apparently even British English uses "targeted".

http://www.future-perfect.co.uk/grammartips/grammar-tip-targetted-targeted.asp

I give up :D

@@ +501,5 @@
> +**409 Conflict**
> +
> +    Another client is currently making changes to the resource targetted by
> +    a write request (PUT, POST, DELETE) or to a related resource.  The write
> +    request has been rejected to esure that conflicting edits do not lead to

s/esure/ensure

@@ +509,5 @@
> +    introduced by other clients.
> +
> +    This response may include a *Retry-After* header indicating the time at
> +    which the conflicting edits are expected to complete.  Clients should
> +    wait until at least this time before retrying the request.

What if there's no Retry-After?

Comment 8

[Ryan Kelly [:rfkelly]]

Since we're supposed to always include enough information to resolve the conflict, I've made Retry-After mandatory.  It's probably better this way anyway, as a sensible value for "default retry-after" may change depending on implementation details of the server.

https://github.com/mozilla-services/docs/commit/4d4cf8d14dbd185c6a0a5337f5c0275ee3ad9174

Comment 9

[James Bonacci [:jbonacci]]

Verified the latest spec here: http://docs.services.mozilla.com/storage/apis-2.0.html