Race condition in DiskStore

[adrianomarto]

I found a race condition between the methods setitem and getitem. Sometimes when the method getitem is called, the file containing the session data is not complete because it is still being written by another thread.

I fixed the problem by adding a recursive lock to the class and modifying the methods setitem and getitem so they acquire the lock before trying to read or write the session file.

[anandology]

That still doesn't solve the issue of half written files. I guess the right way to solve this is by writing to a tmp file first and move the file to actual location.

[adrianomarto]

Anand,
I don't see how a half-written file would happen. In the setitem method, the complete file operation (get path, open, write, close) is performed within the lock.acquire-release guard. No thread works on any store file until the previous store file operation is finished.
Also, I don't see how a tmp could solve the problem. In my opinion, it would only transfer the same problem to a different place.
Regards,
Adriano.

[gmelikov]

@adrianomarto I'm a bit out of context, but if app crashes in the middle of setitem, then you'll have a partially written file (i.e. invalid). So tmp files can help in this case, and there will be one less lock (getitem doesn't need a lock if using tmp files).

[adrianomarto]

@gmelikov, if you look at the code carefully, you will see that the file is closed in the "finally" clause (and it was already like this before my changes). That should suffice to take care of the crash condition, which is not the problem being addressed here.

The problem I am trying to solve happens when a thread tries to read the file when the file is still being written. The problem still happens if you move the file from elsewhere instead of writing the file directly to where it belongs.

Solving the problem by moving the file form a temporary folder would require the moving operation to be atomic, which I don't dare say to the case in all operating systems. On the other hand, solving the problem by using locks is guaranteed to work, as the locking operation is atomic regardless of the operating system.

[gmelikov]

@adrianomarto IIRC now DiskStore isn't crashproof too, finally condition won't do anything, if python crashes on f.write(pickled). Maybe it's the case for another PR.

By default open buffers IO https://docs.python.org/3/library/functions.html#open

the default buffering policy works as follows:
Binary files are buffered in fixed-size chunks; the size of the buffer is chosen using a heuristic trying to determine the underlying device’s “block size” and falling back on io.DEFAULT_BUFFER_SIZE. On many systems, the buffer will typically be 4096 or 8192 bytes long.

[gmelikov]

@adrianomarto https://docs.python.org/3/library/os.html#os.rename

If successful, the renaming will be an atomic operation (this is a POSIX requirement).

[cclauss]

Conflicts: This PR needs to be re-based.

[iredmail]

Fixed in 058672d