Add option to lock cookie files while reading & writing

[kylerippey]

When using the rets gem in multiple processes, it's possible for shared cookie files to become corrupt due to race conditions caused by reading and writing.

I've added the ability to utilize Ruby's flock to lock the files while reading and writing.

[phiggins]

I have two concerns:

1. How does the new option interact with other options? I'll admit not having tons of knowledge of this codebase, but if I set :lock_cookie_files and :cookie_store does everything still work as expected?
2. Reading the IO#flock docs (http://ruby-doc.org/core-2.2.0/File.html#method-i-flock), it doesn't look like there's any sort of built in timeout or anything. Blocking on acquiring a lock indefinitely could end badly. Do we need a timeout? Is there a best practice for this?

[kylerippey]

How does the new option interact with other options? I'll admit not having tons of knowledge of this codebase, but if I set :lock_cookie_files and :cookie_store does everything still work as expected?

Yes, this option will work fine with :cookie_store. :cookie_store is the path to the cookie file, which we set immediately after enabling the locking cookie strategy. At this point the LockingCookieSaver will be used to load the specified cookie file.

Reading the IO#flock docs (http://ruby-doc.org/core-2.2.0/File.html#method-i-flock), it doesn't look like there's any sort of built in timeout or anything. Blocking on acquiring a lock indefinitely could end badly. Do we need a timeout? Is there a best practice for this?

Good point. What about something like this?

def save(io, jar)
  Timeout::timeout(1) { io.flock(File::LOCK_EX) } if io.respond_to?(:flock)
  super
end

def load(io, jar)
  Timeout::timeout(1) { io.flock(File::LOCK_SH) } if io.respond_to?(:flock)
  super
end

This should raise Timeout::Error if it fails to acquire the lock. Since this is taking place inside of a File.open block, Ruby should properly close the file for us.

[phiggins]

That seems good to me. We'll probably want to try this out in production for a bit and see if any issues shake loose though.

[kylerippey]

That seems good to me. We'll probably want to try this out in production for a bit and see if any issues shake loose though.

Added the timeout and a spec.

[dougcole]

do we really need this as an option? What would be the downside to having this always enabled? It seems like a best practice when dealing with files like this. I assume it'll add a little overhead, which might be worth measuring, but I have trouble believing it'd be enough to be a bottleneck.

[kylerippey]

do we really need this as an option? What would be the downside to having this always enabled? It seems like a best practice when dealing with files like this. I assume it'll add a little overhead, which might be worth measuring, but I have trouble believing it'd be enough to be a bottleneck.

Agreed, it should be fine. I was simply concerned with enabling it by default as part of a patch version release. If we're willing to increment the minor version, then I'm happy to make it default.

[kylerippey]

I've permanently enabled the locking behavior.

[dougcole]

At Estately, we worked around this as well as several related problems by storing our metdata in a postgresql database. Potentially overkill for other people, but it works well for us and is easy to do with custom cache and serializer classes.

I'm closing this as we don't need it, but if someone else is interested in it, please chime in and we can re-examine merging it.