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

Refactor file locking code for testing #75

Merged
merged 1 commit into from Jul 14, 2016
Merged

Conversation

vmrob
Copy link
Contributor

@vmrob vmrob commented Jul 14, 2016

No description provided.

@vmrob
Copy link
Contributor Author

vmrob commented Jul 14, 2016

Only question is naming convention of file_lock.py and the function lock_file. This really doesn't provide that much utility right now, but #67 makes good use of this and I think it's useful enough to merge without the other PR.

@vmrob
Copy link
Contributor Author

vmrob commented Jul 14, 2016

I take that back... The FileLock context manager isn't used anywhere. Let me remove that and if it's ever to be used, it should be easy to add it back.

pass
if lock_acquired or datetime.now() > start + timedelta(seconds=timeout):
break
time.sleep(1)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really don't like this way of implementing a timeout. if the timeout is necessary, we should probably do it some other way...

http://stackoverflow.com/questions/5255220/fcntl-flock-how-to-implement-a-timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can work around it, but it's worth mentioning that the suggested method is somewhat global to the process even if it is better.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have any plans or motivation to multithread Needy itself, so I'd definitely prefer it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's about what I thought. Definitely better to just allow multiple single-threaded Needy instances.

@vmrob vmrob force-pushed the file-locking branch 2 times, most recently from c7c8b32 to dc04c5d Compare July 14, 2016 03:08
@vmrob
Copy link
Contributor Author

vmrob commented Jul 14, 2016

Much cleaner.

@vmrob vmrob force-pushed the file-locking branch 2 times, most recently from fc9f6dc to f6660b0 Compare July 14, 2016 03:10
if not self.__blocking:
return None
print('Waiting for other needy instances to terminate...')
fcntl.flock(self.__fd, fcntl.LOCK_EX)
self.__fd = lock_file(self.__path, timeout=None)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could leave off the timeout=None here

@ccbrown
Copy link
Owner

ccbrown commented Jul 14, 2016

Looks good. I'll merge it once Travis approves.

@vmrob
Copy link
Contributor Author

vmrob commented Jul 14, 2016

Removed the timeout=None.

@ccbrown ccbrown merged commit e99e7fd into ccbrown:master Jul 14, 2016
@vmrob vmrob deleted the file-locking branch July 16, 2016 07:55
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

Successfully merging this pull request may close these issues.

None yet

2 participants