Opened 8 years ago

Last modified 3 years ago

#371 accepted defect

I2PSnarkServlet / SnarkManager DirMonitor

Reported by: John Doo Owned by: zzz
Priority: minor Milestone: 0.9.25
Component: apps/i2psnark Version: 0.8.2
Keywords: Cc:
Parent Tickets:

Description

I2PSnarkServlet "Create".equals(action)" already contains a note that a newly created .torrent metainfo file could be read by DirMonitor? before it is completely written to disc (" DirMonitor? could grab this first, maybe hold _snarks lock?")
A simple solution is to create the file with a temporary suffix first (".torren_"), and only rename it to its final name after the stream has been closed:

So my suggestion is:

private static final String TEMP_TORRENT_SUFFIX = ".torren_";

File torrentFile = new File(baseFile.getParent(),baseFile.getName() + TORRENT_SUFFIX);
File tempTorrentFile = new File(baseFile.getParent(),baseFile.getName() + TEMP_TORRENT_SUFFIX);
...
out.close();
tempTorrentFile.renameTo(torrentFile);

The same solution can also be used in FetchAndAdd?, as there is a similar problem: during the timed the fetched .torrent file is copied from the temporary directory to the data directory, DirMonitor? could try to read the partially copied file.

A different way to accomplish the same result is to aquire an exclusive FileLock? on the destination file. The steps are:
File destination = ...;
FileOutputStream? fOut = new FileOutputStream?(destination);
OutputStream? out = new BufferedOutputStream?(fOut);
FileChannel? fc = fOut.getChannel();
FileLock? fileLock = fc.tryLock();non-block
[copy the content from in to out]
fileLock.release();

This could be incorporated in FileUtil?.copy(String source, String dest, boolean overwriteExisting, boolean lockDestination).

Sincerely Yours
John Doo

Subtickets (add)

Change History (5)

comment:1 follow-up: Changed 8 years ago by zzz

  • Milestone set to 0.8.4
  • Owner set to zzz
  • Status changed from new to accepted

Yeah, as you can see from the comments it was already on my list. In my dhtsnark branch scheduled for 0.8.4 I have a fix using locking, but I'm not happy with it because it hangs the GUI for too long. I may be able to make the lock more fine-grained, not sure.

The rename is an idea. I had problems with Java rename on Windows a couple releases ago, it doesn't seem to be reliable, but that may have been from renaming a file over another file.

I'm not familiar with FileLock? but I'll take a look at that too.

So there are several options.

comment:2 in reply to: ↑ 1 Changed 8 years ago by John Doo

Replying to zzz:

Yeah, as you can see from the comments it was already on my list. In my dhtsnark branch scheduled for 0.8.4 I have a fix using locking, but I'm not happy with it because it hangs the GUI for too long. I may be able to make the lock more fine-grained, not sure.

The rename is an idea. I had problems with Java rename on Windows a couple releases ago, it doesn't seem to be reliable, but that may have been from renaming a file over another file.

I'm not familiar with FileLock? but I'll take a look at that too.

So there are several options.

The File.renameTo(File) method states in its doc that it doesn't guarantee to succeed if the destination file already exists. But as in this particular case there is a check whether the .torrent file already exists, this shouldn't be a problem here. For me this solution works fine on Windows.

comment:3 Changed 7 years ago by str4d

  • Milestone changed from 0.8.12 to 0.8.14

comment:4 Changed 7 years ago by zzz

  • Milestone 0.8.14 deleted

Milestone 0.8.14 deleted

comment:5 Changed 3 years ago by zzz

  • Milestone set to 0.9.25

In the previous years:

  • FileUtil?.rename() has several improvements
  • DirMonitor? has locking with other parts of snark
  • Java FileLock? only works within Java, doesn't prevent external changes
  • Additional changes in the i2p.i2p.zzz.dirmon branch, scheduled for propping in 0.9.25, including a second rename() within i2psnark/, in case /tmp is on a different filesystem.
Note: See TracTickets for help on using tickets.