Repository navigation
Conversation
This makes me inclined to bump the milestone early! 😄 |
jtulach
left a comment
jtulach
left a comment
There was a problem hiding this comment.
The original intention of FileLock was to provide access to real filesystem locking in times where Java lacked the API to access the operating system locks. That's no longer true, we should be using real java.nio.channels.FileLock instead of .LCK files.
However there is no reason to block the changes in this PR while waiting for someone to implement java.nio.channels.FileLock based locking. The changes here are just small refactorings of existing code with the intent to disable the provider via standard metaInfServices mechanism.
Go on, but please document the use-case properly. So we know what kind of obligations are created and whether they can be changed in the future.
| # under the License. | ||
|
|
||
| #suppress the file-based hard locks: | ||
| #-org.netbeans.modules.masterfs.filebasedfs.fileobjects.LockForFile$FileBaseHardLockRegistry |
There was a problem hiding this comment.
I'd like the use-case to be described in platform/masterfs/arch.xml - e.g.
<api group="lookup" type="export" name="Disable .LCK files" category="devel">
To disable.... remove this class ... this is an API under development and may be replaced or changed in the future.
</api>
Also add versioning and a note into apichanges.xml.
| } | ||
|
|
||
| @ServiceProvider(service=HardLockRegistry.class, position=100) | ||
| public static final class FileBaseHardLockRegistry implements HardLockRegistry { |
There was a problem hiding this comment.
- Make it a top level class, maybe?
- The name of the class becomes an API ...
- avoiding
$in the name ... simplifies the API
| } | ||
|
|
||
| /*not private for tests*/ | ||
| public static interface HardLockRegistry { |
There was a problem hiding this comment.
- the name of the class is an API
- one needs it to create appropriate
META-INF/servicesfile
- one needs it to create appropriate
- make the class top-level, please!
- I would even suggest to give it a dedicated package
- maybe
org.netbeans.modules.masterfs.locking?
- maybe
- update the Javadoc it seems out of date given the new use-case created by this PR
1cae211 to
c05aba5
Compare
|
I have rebased to the current master, and reflected the review comments. I didn't introduce a new package, as there are package-private reference to and from |
jtulach
left a comment
jtulach
left a comment
There was a problem hiding this comment.
- There is a copy&paste error in
<summary>to fix. - additional links would help readability of the
ant javadocgenerated HTML
| <changes> | ||
| <change id="disable.hard.locks"> | ||
| <api name="masterfs"/> | ||
| <summary>Split image handling into UI module</summary> |
There was a problem hiding this comment.
"Split image handling into UI module?" - probably copy and paste error. Try to generate ant javadoc and then read thru the generated pages.
| <code>CvsFileSystem</code> will be now provided by <code>MasterFileSystem</code>. | ||
| There will be guaranteed that there won't be more instances of FileObject | ||
| addressing one <code>java.ioFile</code>. | ||
| <api name="Disable .LCK files" group="lookup" type="export" category="devel"> |
There was a problem hiding this comment.
Short, but exactly to the point. Thanks. This entry generates a stable <a name reference. I suggest to link to it from various places.
| <author login="jlahoda"/> | ||
| <compatibility binary="compatible" source="compatible" deletion="no"/> | ||
| <description> | ||
| Adding an ability to disable hard-locks using files on the file system. |
There was a problem hiding this comment.
Add a link to the "a name" reference to https://github.com/apache/netbeans/pull/6639/changes#r4344525386 - it would be something like:
<a href="@TOP@/architecture-summary.html#disable-.lck-files">| import java.io.IOException; | ||
| import org.openide.util.Lookup; | ||
|
|
||
| /** A pluggable hard lock handler. */ |
There was a problem hiding this comment.
Add a link to https://github.com/apache/netbeans/pull/6639/changes#r4344525386 - e.g. something like:
<a href="@TOP@/architecture-summary.html#disable-.lck-files">5 participants
NetBeans FileSystems have a notion of file locking. And, when two (or more) files with the same name, but different paths, are locked, there are lock file written next to the files on the OS file system. Investigating this, it seems the reason is to detect multiple lock for the same "real" file available under multiple paths. That may happen e.g. by using symbolic links,
mount --bindor network files. Materializing the lock files on the OS file system should allow to detect multiple lock attempts for the same "real" file.Not sure how important this is, but I don't recall any complaints about this while running a NB platform app or NB-based IDE, so keeping the behavior for these usecases would make sense to me. Note the NB FileSystems don't model the lock files at all, so they are invisible when using the NB FS, and hence mostly invisible inside the IDE.
But, in the context of an VS Code extension, the lock files are problematic. It is fairly easy to produce them, and they are visible to the user. So, preferably, the lock files should not be created in the extension(s). Please see:
#4962
So, my proposal herein is to abstract the hard-lock creation, and create either the lock files, or keep hard-locks in memory only. Where NB would use the former, and the VS Code extension(s) the latter.
This means the VS Code extension(s) won't be able to detect some double-locking of the same file using different paths, but I think that is OK/acceptable. I've used
Path.toRealPath(), so symbolic links are resolved, and hence double-locking across sym-linked directories should still work. We'll see how that goes, if this proves to be too resource intensive, we might need to drop that. Double-locking a file that is mounted multiple times won't be detected.For the record, I am not a real expert on NB FS, and so this patch is fairly dangerous. It may need some baking time on master, at least for NB IDE.