Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider replacing fcntl with portalocker for Windows support #53

Closed
jpmckinney opened this issue Apr 9, 2020 · 7 comments
Closed

Consider replacing fcntl with portalocker for Windows support #53

jpmckinney opened this issue Apr 9, 2020 · 7 comments

Comments

@jpmckinney
Copy link
Contributor

It's not available on Windows.

@Bjwebb
Copy link
Member

Bjwebb commented May 28, 2020

It's used for file locking in

lib-cove/libcove/lib/common.py

Lines 1241 to 1270 in 0bc725b

if os.path.exists(local_org_ids_file):
with open(lock_file, "w") as lock:
fcntl.flock(lock, fcntl.LOCK_EX)
fp = open(local_org_ids_file)
org_ids = json.load(fp)
fp.close()
fcntl.flock(lock, fcntl.LOCK_UN)
date_str = org_ids.get("downloaded", "2000-1-1")
date_downloaded = datetime.datetime.strptime(date_str, "%Y-%m-%d").date()
if date_downloaded != today:
get_remote_file = True
else:
get_remote_file = True
first_request = True
if get_remote_file:
try:
org_ids = requests.get(orgids_url).json()
org_ids["downloaded"] = "%s" % today
with open(lock_file, "w") as lock:
fcntl.flock(lock, fcntl.LOCK_EX)
fp = open(local_org_ids_file, "w")
json.dump(org_ids, fp, indent=2)
fp.close()
fcntl.flock(lock, fcntl.LOCK_UN)
except requests.exceptions.RequestException:
if first_request:
raise # First time ever request fails
pass # Update fails

We could take a different approach to this, but I'm not quite sure how much work it would be.

@jpmckinney
Copy link
Contributor Author

Why do we need to lock the file, though? In what scenario would there be simultaneous writes?

@Bjwebb
Copy link
Member

Bjwebb commented May 29, 2020

This gets called from cove-ocds web requests, where we may have multiple processes/threads calling this function at once.

@jpmckinney jpmckinney changed the title Is fcntl strictly necessary? Consider replacing fcntl with portalocker for Windows support May 29, 2020
@jpmckinney
Copy link
Contributor Author

Aha. Renamed issue and found this library that can perhaps be used to support multiple operating systems: https://pypi.org/project/portalocker/

@benvand
Copy link
Contributor

benvand commented Feb 13, 2021

My proposal for safely removing the file locking

The 2 race conditions to account for are:

Process A checks for the existence of the file finds none
Process B checks for the existence of the file finds none
Process A/ B writes new file
Process A/ B writes new file

Process A checks for the existence of the file finds none
Process A opens file for writing
Process B checks for the existence of the file
Process B opens the file
Process B reads nothing
Process A writes to file

In the first case we don't care because the data is never mutated and gathered from the same source.
In the second case the trick is to write the file before you ever open it :) Create a temp file, write to that, then rename it.

This wouldn't work in cases where data is mutated, but it does in ours.

Also added tests

@jpmckinney
Copy link
Contributor Author

Closed by #74

@jpmckinney
Copy link
Contributor Author

@Bjwebb You can close this issue to declutter issue tracker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants