Excessive locking

[MarkCiliaVincenti]

I looked at your locking mechanism out of interest because I wrote a locking library myself. I noticed that you may be locking quite excessively and I'm not sure if it's done purposely. For example, if you want to lock on a specific file or on AllStreams, you would not be allowing concurrency for different files/streams. If you don't want this behaviour, you need to consider using a keyed locking library instead, such as the one I made at https://github.com/MarkCiliaVincenti/AsyncKeyedLock which also allows pooling helping to reduce allocations.

[ManlyMarco]

I came across this on some ocassions I believe, it's possible to deadlock in some cases if you try to run some of the API synchronously (I believe it was MoveFileAsync). I don't have anything reproductible though and as long as everything is done asynchronously it doesn't seem to be a problem.

[MarkCiliaVincenti]

But it's not exactly related to synchonously/asynchronously but rather with regards to concurrent asynchronous tasks.

[alanmcgovern]

For example, if you want to lock on a specific file or on AllStreams, you would not be allowing concurrency for different files/streams.

The current implementation should allow concurrent usage of multiple filestreams for a single file, and concurrent usage of filestreams from different files. The intent of the implementation is:

1. The library is configured to allow up to MaxOpenFiles filestreams to be open. This limit is enforced by ensuring only that many readers, or writers, can be active at a specific point in time:

   monotorrent/src/MonoTorrent.PieceWriter/MonoTorrent.PieceWriter/DiskWriter.cs

   Lines 169 to 175 in 86fa0a2

   	using (await Limiter.EnterAsync ()) {
   	(var writer, var releaser) = await GetOrCreateStreamAsync (file, FileAccess.Read).ConfigureAwait (false);
   	using (releaser)
   	if (writer != null)
   	return await writer.ReadAsync (buffer, offset).ConfigureAwait (false);
   	return 0;
   	}

   . This async semaphore is entered before a filestream is claimed, and is released when the read/write operation is complete.

2. Assuming MaxOpenFiles is a large number and there are multiple readers/writers and there's only 1 file [0] in the torrent. Each concurrent reader/writer will need to claim a filestream. In order to do this, it a temporary lock is taken on the AllStreams locker to synchronize access to the underlying collection of available streams. With this lock is held the list of already-open streams is scanned for an available one, and if one is found the stream is locked and returned. At this point the lock on AllStreams is released. Any other threads which need a stream for this file are unblocked now.

3. The actual read/write operation is executed with just two locks held: the ratelimiting semaphore from point 1, and the lock on the specific filestream from point 2. Once the read/write operation completes, the lock on the filestream will be released and then the ratelimiter will be released too and things are done.

[0] if there are multiple files in the torrent, then they'll all lock on a different AllStreams object, which means multiple threads could claim streams at the same time.

There is also an implicit assumption that access to the Streams dictionary (

monotorrent/src/MonoTorrent.PieceWriter/MonoTorrent.PieceWriter/DiskWriter.cs

Line 80 in 86fa0a2

Dictionary<ITorrentManagerFile, AllStreams> Streams { get; }

) is not multi-threaded, and that's guaranteed by virtue of the code invoking the DiskWriter methods being 'correct'.

Which part of this were you suggestingchanging the synchronization model for?

That said, i'll still check out the code you suggested! I'm curious to see how others approach these sorts of problems!