Skip to content

Fix for good DefaultTrackingFileManager #1800#1812

Closed
martins-avots wants to merge 3 commits intoapache:masterfrom
martins-avots:master
Closed

Fix for good DefaultTrackingFileManager #1800#1812
martins-avots wants to merge 3 commits intoapache:masterfrom
martins-avots:master

Conversation

@martins-avots
Copy link
Copy Markdown
Contributor

@cstamas Is the usage of FileLockNamedLockFactory correct for DefaultTrackingFileManager#read?

@cstamas
Copy link
Copy Markdown
Member

cstamas commented Mar 24, 2026

Yes it is, but observe this: #1800 (comment)

Summary:

  • maven2 locked the file itself
  • maven3.x continued doing the same to preserve backward compat (so until this point they could co-exist working on same local repo)
  • maven3.10, if change to named locks (they do not lock the subject file, they are pure "advisory locking") this breaks

@cstamas
Copy link
Copy Markdown
Member

cstamas commented Mar 24, 2026

Check out my counter proposal: #1814

@martins-avots
Copy link
Copy Markdown
Contributor Author

martins-avots commented Mar 26, 2026

maven3.10, if change to named locks (they do not lock the subject file, they are pure "advisory locking") this breaks

Isn't FileLockNamedLock and DefaultTrackingFileManager's basically the same type of lock aka FileChannel#lock where shared can be set freely?

So instead of if (lock.lockExclusively(FILE_LOCK_TIMEOUT, SECONDS)) {
I would do if (lock.lockShared(FILE_LOCK_TIMEOUT, SECONDS)) { for
DefaultTrackingFileManager's public Properties read(Path path) {?

For public Properties update(Path path, Map<String, String> updates) {
and public Properties update(Path path, Map<String, String> updates) { lockExclusively should still be the correct lock?

Check out my counter proposal: #1814

So this means I should get the factory like this?

    @Inject
    public DefaultTrackingFileManager(FileLockNamedLockFactory factory) {
        [...]
    }

@cstamas
Copy link
Copy Markdown
Member

cstamas commented Mar 27, 2026

Yes, you can do that too, but there were other refactorings needed, also in same PR.

@cstamas
Copy link
Copy Markdown
Member

cstamas commented Mar 27, 2026

@martins-avots am closing this PR, what we wanted is merged. Thanks!

@cstamas cstamas closed this Mar 27, 2026
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.

2 participants