race to complete data write

[glycerine]

It appears that store.Persist() (and store.Close() ) return before persistence to disk is actually completed.

This creates a race and causes a simple write-then-read test (https://github.com/glycerine/bulletin/blob/master/bulletin_test.go#L10) to fail, unless a print is inserted to slow down the test code.

to reproduce: clone https://github.com/glycerine/bulletin and run go test -v

go test -v 
=== RUN   Test001BulletinPersistanceToDisk

  read/write to persistent disk should work ✘


Failures:

  * /Users/jaten/go/src/github.com/glycerine/bulletin/bulletin_test.go
  Line 39:
  Expected: '[]byte{0x61, 0x20, 0x76, 0x61, 0x6c, 0x75, 0x65}'
  Actual:   '[]byte(nil)'
  (Should resemble)!


1 assertion thus far

--- FAIL: Test001BulletinPersistanceToDisk (0.00s)
FAIL
exit status 1
FAIL    github.com/glycerine/bulletin   0.014s

commenting in the line https://github.com/glycerine/bulletin/blob/master/bulletin.go#L58 slows down the test, causing it to succeed on my mac book. Hence the code is timing finicky. It should not be. I expected that once Persist() or Close() completed, the data is safely on disk. However, inspection of disk reveals no files written before process shutdown. Hence there is background async processing going on that should be synchronous with the completion of Persiste() and/or Close()

[hisundar]

Hi Jason, Thanks for identifying this race condition. This is actually a known aspect, in fact if you further investigate you will notice that store.Persist() also has racy behavior with multiple parallel invocations.
The short answer that that the store.Persist() is not meant to be invoked externally unless the caller explicitly knows exactly what it entails. moss store is meant to be used via the OpenStoreCollection() api and using the Collection interface for all application operations. This is the safest and possibly the simplest way to use moss. Note that collection.Close() does in fact wait for all background persister go routines to complete, unlike the store.Close().
The apis of store are meant only for advanced users.
From my understanding, this race likely won't be treated as an issue as per design.

[glycerine]

Note that collection.Close() does in fact wait for all background persister go routines to complete

This claim appears to be false because the test already does call collection.Close(), and yet the race is there...
https://github.com/glycerine/bulletin/blob/master/bulletin.go#L122

[hisundar]

Well, the test also invokes store.Persist() ..
https://github.com/glycerine/bulletin/blob/master/bulletin.go#L119
As I mentioned earlier collection.Close() only waits for all background persister go routines, which is why it isn't recommended to invoke store.Persist() directly.

[glycerine]

Hi Sundar @hisundar,

Thanks for your comments.

Please recommend a fix.

For example, commenting out the store.Persist() does not fix the race, nor allow the collection.Close() to succeed before process shutdown.

Can this be fixed from the client, or does it identify a structual bug in the moss library which requires redesign?

[mschoch]

Hi @glycerine

As @hisundar said, moss is functioning as designed in this case, as the use case we built it for did not require data to be persisted to disk before these methods return. It doesn't mean that moss doesn't work, it just means that it doesn't work in the way you're trying to use it.

That said, I disagree with @hisundar as to whether or not this should be addressed. I believe this issue is already open here: #10

The honest answer is that we haven't decided if we want to support this use case or not. At the moment moss is very simple, and adding the code required for durable persistence will complicate things. But unless we do this, moss isn't suitable as a general purpose key/value store.

At the moment there is nothing a client can do to guarantee data has been persisted to disk (without resorting to hacks like waiting for arbitrary periods of time and possibly checking stats counters). But again, this is consistent with the design. It is simply a limitation we accepted from the very beginning because it works fine for our use case without it.

[hisundar]

Hi Jason, looking at your test case it appears that you only wish to wait for data to be durably written before close. For this simple use case, given the code that we have today in moss, could you please try adding a function like this..
https://github.com/couchbase/moss/blob/master/dgm_test.go#L152
and invoking it instead of the b.store.Persist() that you commented out ?

As @mschoch indicated, it is not that we do not wish to add a durable persistence model, just that at this point we are not sure if adding code for doing that will complicate things or impact performance.
That said, for several use cases such as yours where eventual persistence with guaranteed durability on close is acceptable, the function like the one above should work as of today.

[hisundar]

While we are on this topic, @steveyen, @mschoch - can we just perhaps expose a function like the one above,
func (c Collection) waitForPersistence(opts WriteOptions{}) error
which simply polls a stat with exponential backoff.
It won't be the most elegant solution for durability, but it would guard against data loss and could very likely work for several use cases like this one from @glycerine?

[steveyen]

It won't be the most elegant solution for durability

hehe -- indeed! I'm having sort of a "oh, yuck!" reaction to that. It would work, but...

One thought is maybe adding something to the persister loop, while it's holding onto the collection lock... so we're sure that no mutations are going to concurrently sneak in... and the persister checks some new wanna_close_only_if_all_clean_please flag...

If it's true, then the persister checks that there aren't any more dirty mutations, somehow runs the close() sequence (easier handwaved than actually implemented due to the possible dead-lock here). That way any concurrent mutation attempts during this time by other goroutines would either succeed (their mutations are persisted) or would get a ErrClosed.

[tleyden]

A nice interface might be to get a handle back when you persist something, which you can optionally block on:

persistStatus := b.store.Persist()
persistStatus.Wait() 

[hisundar]

One thought is maybe adding something to the persister loop, while it's holding onto the collection lock... so we're sure that no mutations are going to concurrently sneak in... and the persister checks some new wanna_close_only_if_all_clean_please flag...

I am not sure how this will bubble up to the user who needs an api that tells them stuff made it to disk? The collection.Close() already does wait for the persister go routine to finish its stuff, are you suggesting that we have a collection option that tells Close() to block until all dirty ops are persisted? If so, that would make durability guarantee only available on close - the user will have no means of enforcing durability in the middle of normal moss operations right?

@tleyden - the problem seems to be one step before your suggestion - we don't yet have a non-racy api to persist or wait for persistence yet.

[steveyen]

If so, that would make durability guarantee only available on close - the user will have no means of enforcing durability in the middle of normal moss operations right?

Indeed - great point. You're right and users will likely want optional, enforced durability in the midst of app usage.

[mschoch]

I think we're focused on the right thing though, the idea should be to get the API right now, even if the implementation is limited for the moment. What we can't do is put in some incorrect API and be stuck with it forever.

My proposal, add a field Durability to the WriteOptions struct. Durability options are something like None, and Fsync. (I think we could/should add 3rd option of Write in the future, but we can omit it for now for simplicity)

In the initial implementation, calling ExecuteBatch with anything other than None returns error NotImplemented.

Then, Collection Close() takes new argument, either WriteOptions or Durability directly. In this method we could support the Fsync option today using the polling method Sundar described above.

Of course we know people also want durability without having to close the collection, but if we take this approach, we focus on the long-term API and a short-term implementation. The risk would be that we get the API wrong, but it feels pretty solid to me.

[steveyen]

@mschoch -- that proposal sounds pretty good to me. Might need a CloseEx(CloseOptions) so that the existing Close() remains stable/compatible.

[glycerine]

If the client wants a notification when a write has been fsynced durably to disk, have the client provide an open channel to be closed when the fsync is complete. That way there's no excess allocation, but the client can be as synchronous as it needs to be. If it needs to, the client can use the channel in its own select with timeout. By not allocating a channel automatically for every request, Moss avoids memory allocations that the client doesn't require. Inside the Moss library, the fast path is a simple check on that nil channel, which will be very cheap when the channel isn't used.

[mschoch]

What advantage does this approach offer over the traditional approach of having durable requests block until the requested durability is satisfied? The other advantage of simply blocking is that we can (and should) return I/O errors to the caller (and not just deliver them to callback as we do today).