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

Option to copy files atomically #3849

Closed
catap opened this issue Feb 4, 2021 · 9 comments · Fixed by #4060
Closed

Option to copy files atomically #3849

catap opened this issue Feb 4, 2021 · 9 comments · Fixed by #4060
Labels
feature features we would like to implement

Comments

@catap
Copy link
Contributor

catap commented Feb 4, 2021

Problem

When you importing a huge flac file to network disk via not so fast network (wifi for example) it requires a big time to complete.

If network disk has an agent that indexes files as soon as it was uploaded — for example default behaviour of Synology — it may start to index file before it is fully uploaded.

As result index will be broken.

The easy way to fix it is make inserting atomic by split it to two steps:

  1. upload file to library with name like $realName.$someTmpPostfix
  2. rename from $realName.$someTmpPostfix to $realName
@sampsyo sampsyo changed the title Move via temporary file Option to copy files atomically Feb 4, 2021
@sampsyo sampsyo added the feature features we would like to implement label Feb 4, 2021
@sampsyo
Copy link
Member

sampsyo commented Feb 4, 2021

Hello! Yes, I can see this being useful as an option. Doing so would need to take some care to ensure that the temporary filename is unlikely to be confused for a complete music file.

@catap
Copy link
Contributor Author

catap commented Feb 4, 2021

@sampsyo I guess the name [name].[somerandomstring].beets should be good ;)

@wisp3rwind
Copy link
Member

If network disk has an agent that indexes files as soon as it was uploaded — for example default behaviour of Synology — it may start to index file before it is fully uploaded.

As result index will be broken.

TBH, that sounds like a bug with the indexing service. Shouldn't it detect when the file is subsequently modified (on completion of the upload) and re-index? Is this really a race condition that beet should workaround?

@catap
Copy link
Contributor Author

catap commented Feb 4, 2021

@wisp3rwind this bug was reported to Synology also ;)

But it is a good practice to make upload atomic via temp file into the same directory.

For example it is the best practice when you're wirking with maildir.

@jackwilsdon
Copy link
Member

I think it's common to prefix temporary files with a . to hide them from the file explorer (and other tools) whilst it is still being copied. e.g. .[name].[somerandomstring].beets - rsync does this.

@catap
Copy link
Contributor Author

catap commented Aug 14, 2021

@jackwilsdon agreed!

@sampsyo any hope that it can be done? If you suggest places in the code where I need to add it, I can do it. But I scary that I may broke something :)

@sampsyo
Copy link
Member

sampsyo commented Aug 14, 2021

A good place to start looking into it would be the utility functions that implement moving and copying, such as beets.util.move.

@catap
Copy link
Contributor Author

catap commented Aug 14, 2021

@sampsyo ok, maybe I'll do it in next few days

But if you can do it before, I'll be very appreciated :)

catap added a commit to catap/beets that referenced this issue Sep 14, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 14, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
@catap
Copy link
Contributor Author

catap commented Sep 15, 2021

I'd love to confirm that #4060 fixed all issues with synology indexing.

catap added a commit to catap/beets that referenced this issue Sep 15, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 15, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 15, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 15, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 15, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 17, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Sep 17, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Oct 28, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Oct 30, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
catap added a commit to catap/beets that referenced this issue Oct 30, 2021
The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature features we would like to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants