Adding lock mechanism to prevent on_disk_cache downloading twice

[VitalyFedyunin]

Stack from ghstack:

• -> Adding lock mechanism to prevent on_disk_cache downloading twice #409

Fixes #144

Differential Revision: D36489060

[VitalyFedyunin]

Doing cleanup now, but principle had to remain. The main problem is the fact that we had to put 'promise' files to avoid downloading twice.

[NivekT]

I think we need to add a dependency to portalocker. I think our options are:

1. Soft dependency - only use the locking mechanism when portalocker is installed, but otherwise the DataPipes here still work without it
2. Semi-soft (?) - raise warnings/error when these DataPipes are used but portalocker isn't used. Maybe in DLv2 when mp is enabled (potentially over-engineering)?
3. Hard dependency - always use it and add it to our list of dependencies

[ejguan]

LGTM, thank you!!

[VitalyFedyunin]

Added as hard dependency.

[VitalyFedyunin]

TODOs:
[ ] Remove lambda for full tests coverage
[ ] Add timeout to make sure we exit after waiting promise for two long (default 2 min), as it could happen when filename_fns are different

[VitalyFedyunin]

Had to add promise concept, to make sure that workers waits 'main' worker to finish downloading/unpacking. To avoid DataLoader starvation we also fetch all file names into infinite buffer.

[ejguan]

Thank you soooo much for adding this fix. I do have one comment on 1-to-n scenario.

[ejguan]

Just want to verify, len(result) should be 1, right?

[VitalyFedyunin]

Nope. I'm creating one additional file inside of _slow_fn. So it would be 'downloaded' file and 'pid' file.

[ejguan]

When the cached op is 1-to-n like decompression from archive, if any decompressed file is missing or has incorrect hash, we can directly return False and no need to check other files IMHO.
There can be a chance that multiple processes are locking different decompressed files for an archive. Then, both processes will run decompression -> racing condition again.

So, I think we should lock over data but not filepaths (data represents the compressed archive in this case). For the process that observes promise file over data, they can directly return True.

WDYT?

[VitalyFedyunin]

Unfortunately data could be url or something else, it is hard to lock on it.

But this situation is covered.

Imagine data generates two file namesL file1 file2.

Initial pass (empty FS) will add two locks file1.promise and file2.promise and will go 'False' route.

Now second (and every next) pass will see that files are missing, but will fail to create promise and go into the 'file exists' route, which will led them to the situation when they are waiting for file1.promise and file2.promise to disappear.

[NivekT]

If it is an URL, is it possible to create and lock root/URL.promise in the file system?

I think we should have a similar lock for HttpReader to prevent multiple processes from downloading the same file? Nevermind

[ejguan]

Oh, I see. file_exists flag is used for processes to recognize this file or parent archives are going to be processed by another process.

[NivekT]

If it is an URL, is it possible to create and lock root/URL.promise in the file system?

I think we should have a similar lock for HttpReader to prevent multiple processes from downloading the same file? Nevermind

[ejguan]

Thanks for your explanation. LGTM

[ejguan]

Oh, I see. file_exists flag is used for processes to recognize this file or parent archives are going to be processed by another process.

[VitalyFedyunin]

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[VitalyFedyunin]

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[VitalyFedyunin]

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.