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

Handle Unwritable Path #47

Closed
wants to merge 2 commits into from
Closed

Conversation

ooglek
Copy link

@ooglek ooglek commented Jan 12, 2019

Description

If the OS cannot open a file because the path is bad, it does not make sense to repeatedly attempt to open the file as it will continue to fail, indefinitely if the -1 default timeout is used, without any feedback to the user.

This modifies the behavior to raise the OSError/IOError exception received on Windows or Unix so FileLock exits rather than a futile infinite loop that will never succeed.

Tests are written to demonstrate the behavior.

@AdorablePotato
Copy link
Contributor

Thank you!
I merged your pull request and made some changes to SoftFileLock: https://github.com/benediktschmitt/py-filelock/tree/ooglek-handle-unwritable-path

I can only run tests on linux at the moment, so I'd appreciate any feedback for other systems.

@mcsimps2
Copy link

mcsimps2 commented Feb 6, 2020

@benediktschmitt I don't see these changes in master and it still seems to be a problem in the latest pip release #60. It looks like the changes are only in the branch ooglek-handle-unwritable-path and not in master.

@hmeine
Copy link

hmeine commented Feb 21, 2020

I am very much interested in this fix. Indeed, we had two situations (after switching from a different lock implementation to filelock) in which SoftFileLock would just hang:

  • a read-only filesystem
  • a non-existant directory

In both cases, one would expect an exception to be raised that allows to discriminate from an existing lock file.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Can you please rebase it on the latest main? You'll need to add some tests for it too, Thanks!

@ooglek
Copy link
Author

ooglek commented Sep 27, 2021

I included a test for the change in the original pull request 2+ years ago. Hopefully it should suffice.

@gaborbernat
Copy link
Member

Oh missed that, still needs a rebase though 😊

@ooglek
Copy link
Author

ooglek commented Sep 27, 2021

@mcsimps2 @hmeine My need for this passed 2 years ago. One of you or someone you know will need to carry on this one-line change if you want it in a release.

Signed-off-by: Filipe Laíns <lains@riseup.net>
@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #47 (895b353) into main (9191150) will decrease coverage by 6.52%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   93.14%   86.61%   -6.53%     
==========================================
  Files           2        2              
  Lines         394      411      +17     
  Branches       25       25              
==========================================
- Hits          367      356      -11     
- Misses         22       49      +27     
- Partials        5        6       +1     
Flag Coverage Δ
tests 86.61% <72.22%> (-6.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/filelock/__init__.py 79.44% <0.00%> (-13.34%) ⬇️
tests/test_filelock.py 92.20% <76.47%> (-1.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9191150...895b353. Read the comment docs.

@FFY00
Copy link
Collaborator

FFY00 commented Sep 27, 2021

I rebased the PR.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

We'll need to fix the tests and linting 👍

@FFY00
Copy link
Collaborator

FFY00 commented Sep 27, 2021

Yes, I had a look and the issues seem to be false positives (unless my english is rusty and there is actually a typo in words flagged).

Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
@gaborbernat
Copy link
Member

Superseded by #96

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.

6 participants