-
Notifications
You must be signed in to change notification settings - Fork 117
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
Multiprocess sharing on .NET Core #27
Conversation
Log.Logger = new LoggerConfiguration() | ||
.WriteTo.File("log.txt") | ||
.WriteTo.File("log.txt", shared: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this from the example, since it's possible people will copy it into their projects and sacrifice performance for no reason.
|
||
lock (_syncRoot) | ||
{ | ||
_mutex.WaitOne(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this and the other WaitOne()
call below, should we add a timeout and fail if the mutex can't be acquired? E.g. a 10s timeout is highly unlikely to be hit, but would at least keep things alive.
Ignore the comment about UWP: it's System.Threading.Thread it rejects. Will update the PR. |
… for mutex acquisition
Alrighty, I think this is ready to go. |
(Does also need a test on Linux. Assumed Travis was churning away as it does for serilog/serilog but not so. Might do that from dev along with macOS.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on the same thing :-) Hope it helps.
Also tests should be corrected at place where
const string InvalidPath = "/\\";
to pass on Linux
In my tests with this perf is about 1.5x slower than atomic_append. On Ubuntu
its about 3x times slower, but it also ~3x times slower with shared=false
} | ||
|
||
// Backslash is special on Windows | ||
_mutex = new Mutex(false, path.Replace('\\', ':') + MutexNameSuffix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better to name it with the full path name to ensure uniqueness and better for crossplat, something like
Path.GetFullPath(path).Replace(Path.DirectorySeparatorChar, ':') + MutexNameSuffix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will do.
SelfLog.WriteLine("Shared file mutex could not be acquired in {0} ms for event emitting", MutexWaitTimeout); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you should also handle AbandonedMutexException
in case of another process crash and also protect against async exceptions with help of return value of WaitOne()
:
bool ownsMutex = false;
try
{
try
{
ownsMutex = _syncRoot.WaitOne(MutexWaitTimeout);
}
catch (System.Threading.AbandonedMutexException)
{
ownsMutex = true;
}
// ...
}
finally
{
if (ownsMutex)
{
_syncRoot.ReleaseMutex();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll look into it 👍
Sorry about that, I should have made a note here - I just picked it up this afternoon when I realised how much file-related work was queued up :-) My vague "10x" was with two processes writing on Windows. When there's contention it really seems to slow down. Probably not noticeable in practice though. Thanks for checking the code, I'll run back through it in the morning. |
I think everything should be covered now. |
Looks good 👍 |
Adds a new configuration fornetcoreapp1.0
, as althoughMutex
is available onnetstandard1.3
I think it's unlikely to work on all platforms targeted by that TFM (UWP doesn't like System.Threading references, for instance).Performance is about 10x slower than atomic append, but otherwise appears to work as far as my limited tests have covered.
Required for serilog/serilog-sinks-rollingfile#37.