Skip to content
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

ConcurrentWrites with atomic file writes on .Net5 platform #4244

Open
snakefoot opened this issue Jan 15, 2021 · 13 comments
Open

ConcurrentWrites with atomic file writes on .Net5 platform #4244

snakefoot opened this issue Jan 15, 2021 · 13 comments

Comments

@snakefoot
Copy link
Contributor

snakefoot commented Jan 15, 2021

.Net5 will automatically use the NetStandard2-dependency, so ConcurrentWrites will depend on global mutex (And the global mutex can only be used between applications using the same service-account).

Could be nice if support for atomic-file-appending could be enabled for the .Net5-platform. And the global-mutex could be used across service-accounts just like on .NetFramework.

@snakefoot
Copy link
Contributor Author

Looks like FileStream-constructor with FileSystemRights is available: dotnet/runtime#30435

@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 15, 2021

But it looks like it requires a dependency:

https://www.nuget.org/packages/System.IO.FileSystem.AccessControl/

Guess it has to be implemented in a seperate Nuget-package. Maybe fix FileTarget so it is easy override the creation of FileStream-object. Then create new ConcurrentFileTarget-nuget-package with support for atomic-writes.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 18, 2021

Consider creating this a ConcurrentFileTarget-nuget-package that depends on these packages for Net5:

https://www.nuget.org/packages/System.IO.FileSystem.AccessControl/
https://www.nuget.org/packages/System.Threading.AccessControl/

And then adjust the vanilla FileTarget so it is easy to override FileStream-handling and Mutex-handling where necessary.

Maybe the same nuget-package could add support for assigning File-Permissions on file-creation (nice-to-have)

@snakefoot snakefoot changed the title Enable atomic file writes on .Net5 platform ConcurrentWrites with atomic file writes on .Net5 platform Jan 19, 2021
@snakefoot
Copy link
Contributor Author

snakefoot commented Feb 3, 2021

Thinking that FileTarget should have 2 methods that ConcurrentFileTarget could override:

  • Stream OpenFileStream(string filename)
  • void ArchiveFileStream(Stream openStream, string currentFilename, string archiveFilename)

Instead of FileTarget working with different FileAppenders, then it should just have two types. One for KeepFileOpen=false (Contains filename) and one for KeepFileOpen=true (Contains filename + stream)

The ConcurrentFileTarget can then make a special Stream, that on dispose will also dispose any mutex-object if necessary. Then the Stream-object become the wrapper for any special writing-behavior (ex. writing in gzip format).

The ConcurrentFileTarget can on ArchiveFileStream cast the openStream-object to its own special stream-type, so it can access the inner-mutex when doing archive.

Something like:

protected override void ArchiveFileStream(Stream openStream, string currentFilename, string archiveFilename)
{
    using (var mutexProtection = ExtractArchiveMutex(openStream))
    {
         base.ArchiveFileStream(openStream, currentFilename, archiveFilename);
    }
}

@snakefoot
Copy link
Contributor Author

See also: dotnet/runtime#53432

@heebtob
Copy link

heebtob commented Aug 10, 2022

I use the global mutex for concurrent writes and create it in advance with the everyone permission that the .Net 6 modules work together with .Net Framework 4.8 modules.
But in some rare cases the mutex rights are set to SYSTEM (inherited from a windows service .Net6) which is a problem for web apps which only have very limited rights. If this happens they are not able to log anymore because they don't have access to the mutex. I'm guessing that in some situations the mutex gets recreated during runtime and if that is done by a .Net 6 module the mutex rights are set to SYSTEM.

I saw that there is a new feature for atomic writes in .Net which could be used instead of the mutex.
What needs to be done to get this working?

@snakefoot
Copy link
Contributor Author

The pretty solution would be a refactoring of NLog FileTarget, so one can override the FileAppender-factory.

So NLog FileTarget gets a virtual protected method that returns a Stream-object from a filename. The default implementation of this method is to use the existing logic from the FileAppender-factory.

Then one can create a new nuget-package that extends the NLog FileTarget and overrides the protected method, and then using p/invoke to call the Win32-API Method-CreateFile and provides the magic parameters that enables atomic-file-appending for the file-stream.

Anyway just a random idea, where one skips the use of mutex.

@heebt
Copy link

heebt commented Jul 11, 2023

I have a question regarding the File Archiver Mutex. My Workaround for .Net 4.8 and .Net 6 to write to the same logfile is to create the file and file archive mutex manually before I initialize NLog. Then the Mutex Security is correct even for .Net 6 modules.

In some rare cases I still have problems with mutex security and then we have to stop all applications that write to that file otherwise the mutex remains and some applications are not able to write to the log file and are completely blocked.

Could it be that NLog recreates the File Archive Mutex at some point?
That would explain that it only occures when a .Net 6 Application recreates the File Archiver Mutex.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jul 11, 2023 via email

@heebt
Copy link

heebt commented Jul 12, 2023

I use the same code as NLog except for the Mutex creation itself.

        public static void checkSharableMutex(String filename)
        {
            if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            {
                CreateSharableMutex("FileArchiveLock", filename);
                CreateSharableMutex("FileLock", filename);
            }
        }

        private static Mutex CreateSharableMutex(string mutexNamePrefix, string filename)
        {
            var name = GetMutexName(mutexNamePrefix, filename);

            return ForceCreateSharableMutex(name);
        }

        private static Mutex ForceCreateSharableMutex(string name)
        {
            if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            {
                // Creates a mutex sharable by more than one process 
                var mutexSecurity = new MutexSecurity();
                var everyoneSid = new SecurityIdentifier(WellKnownSidType.WorldSid, null);
                mutexSecurity.AddAccessRule(new MutexAccessRule(everyoneSid, MutexRights.FullControl, AccessControlType.Allow));

#if NET48
                // The constructor will either create new mutex or open
                // an existing one, in a thread-safe manner
                return new Mutex(false, name, out _, mutexSecurity);
#else
                return MutexAcl.Create(false, name, out _, mutexSecurity);
#endif
            }

            return new Mutex(false, name, out _);
        }

@snakefoot
Copy link
Contributor Author

snakefoot commented Jul 12, 2023

Think you should keep the Mutex-objects alive, so the Mutex-creation is not "released"/"cleared" unexpectedly at garbage collection:

        private static Mutex _fileLock;
        private static Mutex _fileArchiveLock;

        public static void checkSharableMutex(String filename)
        {
            if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            {
                _fileLock = CreateSharableMutex("FileLock", filename);
                _fileArchiveLock = CreateSharableMutex("FileArchiveLock", filename);
            }
        }

@vuplea
Copy link

vuplea commented Sep 7, 2023

Hi @snakefoot,

I was wondering if you would add more detail to the following comment:
#4242 (comment)

Guess my thoughts about splitting FileTarget into two, seems to be the only way for Net5.0.

Why ForceCreateSharableMutex couldn't be implemented with MutexAcl to act like on .NET Framework?
A runtime check of Windows (IsOSPlatform), on the NETSTANDARD branch, would only execute on .NET Core 2/3/5/6 Windows runtimes, which all support MutexAcl (with System.Threading.AccessControl 5.0.0).
.NET Framework would use the existing new Mutex(..., mutexSecurity) constructor, while .NET Core non-windows would execute the new Mutex(false, name) constructor.

The downside is adding System.Threading.AccessControl as a PackageReference to NLog, is this the problem?
Extra target frameworks net6.0 and net6.0-windows would allow different dependencies depending on the platform, not sure if this is one of the directions you see in the future.

@snakefoot
Copy link
Contributor Author

NLog v6 will extract FileTarget into its own nuget-package, and then everything is possible.

NLog v5 can open doors on FileTarget, so one can create an extension nuget-package, that adds support for atomic-file-append on Windows / Linux along with correct mutex-handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants