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

Add copy methods to temporary files #8889

Merged
merged 4 commits into from Jan 2, 2019

Conversation

Projects
None yet
4 participants
@marcospereira
Copy link
Member

marcospereira commented Dec 17, 2018

Fixes

Fixes #8885.

Purpose

Add copyTo methods to temporary files and better document the differences between copying and moving files.

References

Depends on #8891.

@wsargent
Copy link
Member

wsargent left a comment

Nice work! Maybe add a test that tests out delete on a moveTo vs a copyTo?

wsargent and others added some commits Dec 20, 2018

Update documentation/manual/working/javaGuide/main/upload/code/javagu…
…ide/fileupload/controllers/HomeController.java

Co-Authored-By: marcospereira <marcos.silva@gmail.com>
Update documentation/manual/releases/release27/migration27/Migration2…
…7.md

Co-Authored-By: marcospereira <marcos.silva@gmail.com>
@marcospereira

This comment has been minimized.

Copy link
Member Author

marcospereira commented Dec 20, 2018

Maybe add a test that tests out delete on a moveTo vs a copyTo

I thought about it but decided not to add a test for that because as I understand, this behavior is platform specific, isn't?

WDYT?

@wsargent

This comment has been minimized.

Copy link
Member

wsargent commented Dec 20, 2018

You can still add a test that only runs on a Unix based system...

@marcospereira

This comment has been minimized.

Copy link
Member Author

marcospereira commented Dec 20, 2018

@wsargent added a new test: 00d23f2

I think ensuring that deleting the source file does not impact the destination is more important for copyTo. So only adding test for that.

@mkurz

mkurz approved these changes Dec 21, 2018

Copy link
Member

mkurz left a comment

LGTM

@marcospereira marcospereira merged commit f791b00 into playframework:master Jan 2, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@marcospereira marcospereira deleted the marcospereira:fix-8885-copy-tmp-files branch Jan 2, 2019

}
}
```

This comment has been minimized.

@mkurz

mkurz Jan 5, 2019

Member

@marcospereira This Java/Scala codesnippets do not render correctly when running the docs locally. Looks like the empty lines do make problems when using this style. Either you have to put this snippets into separate files or we have to fix the empty line problem (which would be nice as well).

This comment has been minimized.

@mkurz

renatocaval added a commit that referenced this pull request Jan 7, 2019

Add copy methods to temporary files (#8889)
* Add copy methods to temporary files

* Update documentation/manual/working/javaGuide/main/upload/code/javaguide/fileupload/controllers/HomeController.java

Co-Authored-By: marcospereira <marcos.silva@gmail.com>

* Update documentation/manual/releases/release27/migration27/Migration27.md

Co-Authored-By: marcospereira <marcos.silva@gmail.com>

* Add new test
Until Play 2.5, `moveTo` method was actually making a copy of the file to the destination and deleting the source. There was a subtle change in Play 2.6 where the file was instead being moved atomically depending on certain conditions. For such cases, both the source and destination end up using the same [`inode`](https://en.wikipedia.org/wiki/Inode) and then deleting the source implies that the destination will be deleted too.

To make the API more clear around this, there are now `moveTo` and `copyTo` methods where `copyTo` always create a copy that does not share the same `inode`. So, if the application is configured to clean up temporary files (see documentation for [[Scala|ScalaFileUpload#Cleaning-up-temporary-files]] or [[Java|JavaFileUpload#Cleaning-up-temporary-files]]) and you want to retain the destination, then use `copyTo` instead of `moveTo`. For example:

This comment has been minimized.

@mkurz

mkurz Jan 16, 2019

Member

What I don't understand here: If I use moveTo/atomicMoveWithFallback to move the file out of the play temp folder, would the file reaper still delete that file? I mean the file reaper just lists the files in the play temp folder and deletes them (if old enough), so it would not access/see the file, because it is not in this folder anymore - or not? Therefore the inode would not matter for such a case or not?

This comment has been minimized.

@mkurz

mkurz Jan 16, 2019

Member

But (like just described in my other comment) as soon as there is no reference to the TemporaryFile instance anymore (on which moveTo/atomicMoveWithFallback was called), the "new" file (same inode, but out of the play temp folder) will be deleted, because we hold a reference on it and therefore finalizeReferent gets called for that file?

case _: FileAlreadyExistsException => to
}

temporaryFileCreator.create(destination)

This comment has been minimized.

@mkurz

mkurz Jan 16, 2019

Member

Another question about this line:
If I copy the file out of the play temp folder the file reaper will not see the new file anymore (see my other comment), meaning the reaper will not delete the new file. However as soon there is no reference anymore to the TemporaryFile instance (on which copyTo was called) the new file will be deleted because of this line here the finalizeReferent method kicks in (for the source file as well as for new file)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment