Make sure all stores are closed if an error occurs with one of them.

[davedoesdev]

Allows two processes trying to access the same database at the same time to
backoff and retry until the other one releases it.
Otherwise one can end up with some of the database's stores and the other one
with the rest, meaning neither can open it.

Note: It doesn't seem there's test infrastructure for adapter-specific tests.

[calvinmetcalf]

I like it, a few, questions/nits

• you should probably call your method something besides __close, as we already have a _close that's just confusing, probably want to call it something of the format descriptionClose
• is this the kind of thing hyperleveldb was designed to deal with?
• is there anything to prevent one process from dying and leaving its lock?

[davedoesdev]

@calvinmetcalf

Yes, I'll rename __close

From my reading (http://hackingdistributed.com/2013/06/17/hyperleveldb/) it seems hyperleveldb achieves better use of multiple threads within a process but not the ability to write from multiple processes.

On linux, leveldb uses fcntl advisory locks:

static int LockOrUnlock(int fd, bool lock) {
  errno = 0;
  struct flock f;
  memset(&f, 0, sizeof(f));
  f.l_type = (lock ? F_WRLCK : F_UNLCK);
  f.l_whence = SEEK_SET;
  f.l_start = 0;
  f.l_len = 0;        // Lock/unlock entire file
  return fcntl(fd, F_SETLK, &f);
}

fcntl man page says:

As well as being removed by an explicit F_UNLCK, record locks are automatically released when the process terminates or if it closes any file descriptor referring to a file on which locks are held.

It looks like LockFile is used on Windows. MSDN says:

If a process terminates with a portion of a file locked or closes a file that has outstanding locks, the locks are unlocked by the operating system.

I don't know about Mac.

[calvinmetcalf]

Thanks, a mac is going to be the same as Linux. I'm not sure if a test is
possible (within reason) as you'd have to have multiple processes running
and hitting the same dB in a coordinated manner. Others might have ideas.
On Dec 24, 2013 6:41 AM, "David Halls" notifications@github.com wrote:

@calvinmetcalf https://github.com/calvinmetcalf

Yes, I'll rename __close

From my reading (http://hackingdistributed.com/2013/06/17/hyperleveldb/)
it seems hyperleveldb achieves better use of multiple threads within a
process but not the ability to write from multiple processes.

On linux, leveldb uses fcntl advisory locks:

static int LockOrUnlock(int fd, bool lock) {
errno = 0;
struct flock f;
memset(&f, 0, sizeof(f));
f.l_type = (lock ? F_WRLCK : F_UNLCK);
f.l_whence = SEEK_SET;
f.l_start = 0;
f.l_len = 0; // Lock/unlock entire file
return fcntl(fd, F_SETLK, &f);}

fcntl man page says:

As well as being removed by an explicit F_UNLCK, record locks are
automatically released when the process terminates or if it closes any file
descriptor referring to a file on which locks are held.

It looks like LockFile is used on Windows. MSDN says:

If a process terminates with a portion of a file locked or closes a file
that has outstanding locks, the locks are unlocked by the operating system.

I don't know about Mac.

—
Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1178#issuecomment-31168787
.

[calvinmetcalf]

you don't need a leading underscore at all here, just api.closeStores is what we need.

[davedoesdev]

Sure, thanks. Just wondering why the other api. functions in that file have a leading underscore?

[calvinmetcalf]

the main adapter object has methods without underscore so when the adapters are implemented they add an underscore to their method names to avoid clashes.

[daleharvey]

Sorry for the delay here, back from holidays now so wont be the same delay in future.

I havent looked into the leveldb process semantics and I never realised they already held locks for us, I thought we have far more severe problems there, thats nice :)

I am worried about exposing this in the api though, we havent kept the api entirely symetrical, the http adapter has some differing capabilities, but I would prefer not to expose them when we dont need to, in this case theres no reason to expose it right? if so could we change it to just a plain inner function and happy to merge

I do think we want test infrastructure for this, but its going to be hard to do and dont want to block improvements on that, I will have node specific tests in soon but this stuff is going to need a lot of extra work to coordinate processes

[calvinmetcalf]

merged 18177d3