cryptomator / cryptofs

Java Filesystem Provider with integrated encryption
GNU Affero General Public License v3.0
94 stars 35 forks source link

More resilient file size #59

Closed overheadhunter closed 5 years ago

overheadhunter commented 5 years ago

Related to #50 (see bec71bf):

In a multithreaded environment, it is possible that thread 1 opens a file, then thread 2 reads the size of the same file, before thread 1 initialized the file size. This leads to an ISE in:

https://github.com/cryptomator/cryptofs/blob/a0386508712de3018f2e6e9832179bb2b91a9716/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java#L152-L155

Instead, we should return a "best effort" file size:

infeo commented 5 years ago

The question myself is asking now: Is the last case an "exceptional" case? Because we could also throw a checked exception, since the method is not directly called from the NIO-API and just internally used.

I would say yes, therefore throw the exception and adapt the calling methods of OpenCryptoFile::size

overheadhunter commented 5 years ago

Another option would be to return an Optional<Long>, which could then be used via flatMap in here:

https://github.com/cryptomator/cryptofs/blob/a0386508712de3018f2e6e9832179bb2b91a9716/src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributes.java#L56-L58

infeo commented 5 years ago

Is the reason, why it failed, interesting? For debugging i would say yes. By (re)throwing an exception we have a stacktrace and see the reason for the failure. But by design it is ok to call the method without an existing file backing it, therefore no. Addtionally it would reduce boiler plate code.

I tend to use the Exception version, since from a bigger picture we already checked the accessability of a ressource with an Files::readAttributes here in line 91: https://github.com/cryptomator/cryptofs/blob/d904b3ecbc51690d51e262c95d33993ad204a21f/src/main/java/org/cryptomator/cryptofs/attr/AttributeProvider.java#L89-L93

Therefore it is an exception if the files suddenly is not accesible anymore.