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

Support to save/load files to/from sshfs-win volumes #1449

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

carlocastoldi
Copy link
Contributor

Recently my laboratory had to move all its files from a Samba storage server to a linux one, accessible only through SFTP/SSH.
For this reason my colleagues' Windows PCs were forced to use sshfs-win in order to mount the remote storage when loading images from QuPath.

Everything seem to work as intended, however saving projects (and probably other files too) is problematic. This is not a problem of QuPath itself, but rather a know problem of sshfs-win not (yet) supporting Windows' GetFileInformationByHandle to retrieve file identifiers.
These identifiers are actively used by Java's Windows' filesystem provider to check whether two files are the same when requesting for a copy() or move() operation.
All this PR does is to use Files.deleteIfExists() instead of calling copy() and move() with StandardCopyOption.REPLACE_EXISTING. Semantically the result is the same.

I know that the issue is not on QuPath's side, however since the fix is very small on yourside i was hoping you would accept this PR anyway. It may come handy to other people in similar scenarios that don't know how to takle the problem

@carlocastoldi
Copy link
Contributor Author

This issue could also be relevant, but i'm not sure: https://forum.image.sc/t/qupath-project-didnt-save-images-and-annotations-are-wrong/61639/8

Depends how their network drive was mounted!

@petebankhead
Copy link
Member

Thanks @carlocastoldi

The change looks a bit scary to me - although it's possible the existing behavior is already a bit scary. I see the javadocs state the following regarding Files.copy with REPLACE_EXISTING

The check for the existence of the file and the creation of the new file may not be atomic with respect to other file system activities.

Nevertheless, I still feel uneasy about explicitly deleting before attempting any copy or move.... since 'may not be atomic' still gives me some hope that we won't end up in some unfortunate state. Corrupt data files was previously a somewhat common complaint, which has reduced a lot over recent releases.

Is there any easy way for us to replicate the issue? Or, if not, could you give the full stack trace for what exception you get without these changes?

I wonder whether creating a method for copy/move that attempts the old behavior, and reverts to the new one if that fails for some reason.

@carlocastoldi
Copy link
Contributor Author

If atomicity of the operation is what worries you, i could implement a method that does it safely, very similarly to what you do with .qpproj files.

  1. write <file>.tmp (if it already exists, it will just overwrite it). If this fails, you have the previous <file>
  2. rm <file>.backup --if-exists. If this fails, you have the <file.tmp> and <file> as the newest versions
  3. mv <file> <file>.backup (works as we are sure the target doesn't exists already). If this fails, you have the <file.tmp> as the newest version
  4. mv <file>.tmp <file> (same, as the previous operation worked). If this fails, you have the <file.backup> as the backup version
  5. rm <file>.backup, if desired

could you give the full stack trace for what exception you get without these changes?

The "problem" is that there is no exception thrown. It just fails on step 3 and/or 4 because source is identified as the same file as target, so it just does not do anything. As if the target was the source.
This means that a project saved on an SFTP server will always end up with three files, project.qpproj, project.qpproj.backup and project.qpproj.tmp, where the latter is the "real" project.
But it also means that when a project is saving the image data it does the following if there was a previous backup:

  • mv <pathData> <pathBackup> -> FAILS : <pathBackup>now has the older backup saved, while <pathData> still exists.
  • write <pathData> -> dangerous operation: if it fails for whatever reason, the only saved state is the one from an older backup

Is there any easy way for us to replicate the issue?

Not that I know of. You have to connect to an SFTP server with windows to do this.

Finally, i think DefaultProject.java could be the only source file that would need to be modified to improve SSH support from windows. The other operations are internal to QuPath, and I doubt people would install the windows version of QuPath in an Linux server and run it with sshfs-win 😅.

Regarding the copy operation, instead, isn't it only ever used when duplicating an Image? In that case how could there be another file in the new data entry's path? I don't think you evern needed the REPLACE_EXISTING attribute over there.

@alanocallaghan
Copy link
Contributor

I still feel uneasy about explicitly deleting before attempting any copy or move.... since 'may not be atomic' still gives me some hope that we won't end up in some unfortunate state. Corrupt data files was previously a somewhat common complaint, which has reduced a lot over recent releases.

I agree, this feels a bit aggressive to me. If the existing copy may not be atomic, I'd rather try to implement an atomic version than to switch to a "definitely not atomic" version.

If atomicity of the operation is what worries you, i could implement a method that does it safely, very similarly to what you do with .qpproj files.

This sounds good to me, it's important for obvious reasons that user data operations are done as safely as possible.

@carlocastoldi
Copy link
Contributor Author

I agree, this feels a bit aggressive to me. If the existing copy may not be atomic, I'd rather try to implement an atomic version than to switch to a "definitely not atomic" version.

I am afraid that copy operations were never guaranteed to be atomic in mainstream OSes. But I agree that this should be something where chance has no room to play

This sounds good to me, it's important for obvious reasons that user data operations are done as safely as possible.

Alright then, I'll work on it.

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

3 participants