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

remove redundant File.length() check to fix race condition #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gregallen
Copy link

See #47

@belingueres
Copy link
Contributor

IMHO this patch does not FIX the race condition, rather just ignore it. I think the check provides a valuable fail-fast mechanism to detect possible problems (as you just did) but removing it won't solve the race condition. The concurrency issue is at a higher level and it is there where it should be fixed.

@gregallen
Copy link
Author

the issue should be safe to ignore since multiple writers to the same file will each succeed in writing their own full copy of the file (on unix) whereas on windows they will either block or fail early.

If you don't wish to merge please can you suggest a way forward?

  • maven dependencies plugin needs to be marked not thread safe
  • how can the maven plugin goal lock across multi module parallel builds?
  • suggest a test case that triggers the file length test to fail AND the files are not in a good state after all copy goals have completed - I'm not convinced this is possible
  • can the check be changed to instead test the number of bytes written?

@belingueres
Copy link
Contributor

I suggest to translate this discussion to the maven developer mailing list to get more answers.

@gregallen
Copy link
Author

actually, here's another point on why I think the post-copy length check is flawed - it can never be thread safe / concurrency friendly, since there's no telling how much time may pass between completion of the file copy and the length check.

Imagine a couple of not too fanciful scenarios:

  1. There is a directory listener that promptly appends a checksum to files when they appear in the destination directory.
  2. Or, the destination file is over-written several times with different contents - e.g. by an optimizer, or an incrementally newer version.

On these grounds, I think the only sane strategy for checking in plexus-util has to be no stronger that a check on number of bytes written

@slachiewicz
Copy link
Member

@slawekjaranowski wdyt?

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.

3 participants