Skip to content

Commit

Permalink
#5273 Returned to use named mutex, because the named semaphore doesn'…
Browse files Browse the repository at this point in the history
…t support bu UNIX based systems
  • Loading branch information
skoshelev committed Feb 16, 2021
1 parent 235c680 commit b4a113e
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 15 deletions.
20 changes: 11 additions & 9 deletions src/Libraries/Nop.Services/Media/PictureService.cs
Expand Up @@ -578,21 +578,23 @@ public virtual async Task<string> GetPictureSeNameAsync(string name)
if (await GeneratedThumbExistsAsync(thumbFilePath, thumbFileName))
return (await GetThumbUrlAsync(thumbFileName, storeLocation), picture);

//the named semaphore helps to avoid creating the same files in different threads,
//the named mutex helps to avoid creating the same files in different threads,
//and does not decrease performance significantly, because the code is blocked only for the specific file.
using (var semaphore = new Semaphore(1, 1, thumbFileName))
//you should be very careful, mutexes cannot be used in with the await operation
//we can't use semaphore here, because it produces PlatformNotSupportedException exception on UNIX based systems
using (var mutex = new Mutex(false, thumbFileName))
{
semaphore.WaitOne();
mutex.WaitOne();

try
{
//check, if the file was created, while we were waiting for the release of the mutex.
if (!await GeneratedThumbExistsAsync(thumbFilePath, thumbFileName))
if (!GeneratedThumbExistsAsync(thumbFilePath, thumbFileName).Result)
{
pictureBinary ??= await LoadPictureBinaryAsync(picture);
pictureBinary ??= LoadPictureBinaryAsync(picture).Result;

if ((pictureBinary?.Length ?? 0) == 0)
return showDefaultPicture ? (await GetDefaultPictureUrlAsync(targetSize, defaultPictureType, storeLocation), picture) : (string.Empty, picture);
return showDefaultPicture ? (GetDefaultPictureUrlAsync(targetSize, defaultPictureType, storeLocation).Result, picture) : (string.Empty, picture);

byte[] pictureBinaryResized;
if (targetSize != 0)
Expand All @@ -605,18 +607,18 @@ public virtual async Task<string> GetPictureSeNameAsync(string name)
Size = CalculateDimensions(image.Size(), targetSize)
}));

pictureBinaryResized = await EncodeImageAsync(image, imageFormat);
pictureBinaryResized = EncodeImageAsync(image, imageFormat).Result;
}
else
//create a copy of pictureBinary
pictureBinaryResized = pictureBinary.ToArray();

await SaveThumbAsync(thumbFilePath, thumbFileName, picture.MimeType, pictureBinaryResized);
SaveThumbAsync(thumbFilePath, thumbFileName, picture.MimeType, pictureBinaryResized).Wait();
}
}
finally
{
semaphore.Release();
mutex.ReleaseMutex();
}
}

Expand Down
Expand Up @@ -278,11 +278,13 @@ protected virtual async Task SavePictureByVirtualPathAsync(Picture picture, int
if (_fileProvider.FileExists(thumbFilePath))
return;

//the named semaphore helps to avoid creating the same files in different threads,
//the named mutex helps to avoid creating the same files in different threads,
//and does not decrease performance significantly, because the code is blocked only for the specific file.
using var semaphore = new Semaphore(1, 1, thumbFileName);
//you should be very careful, mutexes cannot be used in with the await operation
//we can't use semaphore here, because it produces PlatformNotSupportedException exception on UNIX based systems
using var mutex = new Mutex(false, thumbFileName);

semaphore.WaitOne();
mutex.WaitOne();

try
{
Expand All @@ -302,7 +304,7 @@ protected virtual async Task SavePictureByVirtualPathAsync(Picture picture, int
Size = CalculateDimensions(size, targetSize)
}));

pictureBinaryResized = await EncodeImageAsync(image, imageFormat);
pictureBinaryResized = EncodeImageAsync(image, imageFormat).Result;
}
else
{
Expand All @@ -311,12 +313,12 @@ protected virtual async Task SavePictureByVirtualPathAsync(Picture picture, int
}

//save
await _fileProvider.WriteAllBytesAsync(thumbFilePath, pictureBinaryResized);
_fileProvider.WriteAllBytesAsync(thumbFilePath, pictureBinaryResized).Wait();
}
}
finally
{
semaphore.Release();
mutex.ReleaseMutex();
}
}

Expand Down

5 comments on commit b4a113e

@untiedshoes
Copy link

@untiedshoes untiedshoes commented on b4a113e Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question... using RuntimeInformation.IsOSPlatform, can we not store the platform at startup, then depending on the platform being used, use semaphore or mutex if the platform is Windows/UNIX?

https://docs.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer

@skoshelev
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @untiedshoes. technically it is possible, but we decided not to complicate the code. We also created an issue #5256 and will try to avoid using mutex as and semaphore

@untiedshoes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @untiedshoes. technically it is possible, but we decided not to complicate the code. We also created an issue #5256 and will try to avoid using mutex as and semaphore

Thanks Sergey :)

That being said, as all my clients run on windows, for now (until there's a better solution), am I better off using Semaphore rather than Mutex? I'll give the 'ResourcesLocker' in #5499 a try too.

Craig

@skoshelev
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@untiedshoes.

We are not sure about request #5499, but ticket #5498 says that the proposed solution is not very productive.

if all your users are on Windows, then the semaphore is better since it does not require code synchronization, but you can leave it as it is until the next release, since synchronization via a mutex should not greatly affect performance, since these methods are not often called

@untiedshoes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@untiedshoes.

We are not sure about request #5499, but ticket #5498 says that the proposed solution is not very productive.

if all your users are on Windows, then the semaphore is better since it does not require code synchronization, but you can leave it as it is until the next release, since synchronization via a mutex should not greatly affect performance, since these methods are not often called

Yeah, I did some load testing on it, and although it resulted in less blocking, and I didn't need to increase the max pool size via the connection string, the overall response times where much worse under heavy load.

Okay, for now, I'll use semaphore until the next release :)

Thanks again Sergey :)

Please sign in to comment.