kpreisser / AsyncReaderWriterLockSlim

An async-ready alternative to .NET's ReaderWriterLockSlim.
MIT License
25 stars 7 forks source link

Feature Request: .NET Standard 2.0 Compatibility #2

Closed DonaldAirey closed 5 years ago

DonaldAirey commented 5 years ago

I'm porting your library to .NET Standard 2.0 and I was wondering about the use of the native QueryUnbiasedInterruptTime call. Is there any reason you didn't use the DateTime.Now.Ticks property? Is there some reason it can't be used in the .NET Standard 2.0 port (rather than making a native call, which makes me nervous about using this in a .NET Core product).

markusschaber commented 5 years ago

I'd vote for either DateTime.UtcNow.Ticks or, with potentially higher precision, StopWatch - using non-UTC times like DateTime.Now leads to all kinds of strange problems...

kpreisser commented 5 years ago

Hi, thanks for logging the issue!

Note that the code is already using Stopwatch to measure the time on non-Windows OSes. On Windows, Stopwatch uses the QueryPerformanceCounter() function, but this includes also the time the machine has spent in standby/hibernation (the same happens with Environment.TickCount). However, Wait/Sleep methods that take a timeout (like Thread.Sleep() or SemaphoreSlim.Wait()) don't count that time. (DateTime.UtcNow.Ticks has the same issue, and I think it is also generally not a good choice because it has a low resolution and is affected by changes to the current system time.)

For example, if you start a Stopwatch and then call Thread.Sleep(60000) to sleep for 1 minute, then if the PC goes to standby for 5 minutes sometime within the Sleep() call, the Stopwatch will report 00:06:00 as elapsed time after Sleep() returns. This would mean that the methods on the AsyncReaderWriterLockSlim that need to measure the time to calculate the remaining timeout (like TryEnterWriteLock()) might not work correctly when the machine goes to standby/hibernation and return false too early. In contrast, QueryUnbiasedInterruptTime() does not include that time, which is the reason I'm using it on Windows.

Note that the API call is guarded by a RuntimeInformation.IsOSPlatform(OSPlatform.Windows) check, so there shouldn't be any problem using the .NET Standard target of the library because on non-Windows OSes the .NET-provided Stopwatch is used (however, I have not yet checked the if Stopwatch on other OSes like Linux/macOS has the same behavior as on Windows). The only problem that comes to my mind is if using a UWP app on Windows, which might not have access to native APIs in every case. I will need to check this case.

Thanks!

DonaldAirey commented 5 years ago

This comment gives me pause:

                    //// TODO: Check how to correctly get an unbiased timestamp on UNIX.
                    //// Currently we simply use the Stopwatch.

And then we have the actual Stopwatch which is part of the Diagnostics library. I'm a little nervous about using Diagnostic classes for production code.

What problems have you had or do you anticipate with the DateTime.UtcNow.Ticks property?

kpreisser commented 5 years ago

Hi,

What problems have you had or do you anticipate with the DateTime.UtcNow.Ticks property?

As said, DateTime.UtcNow.Ticks has the same issue as Stopwatch in that it also counts time that the system spent in sleep/hibernate state (because it simply uses the current system time). Additionally, DateTime is affected by changes to the system time, which doesn't make it suitable for measuring the elapsed time in order to calculate the remaining wait time; so Stopwatch should be preferred over DateTime.

For example, on Windows the time is synchronized with NTP on a regular basis, which means time might be adjusted by a few seconds in that events. Also, a user can always change the system time to any value.

For the same reasons, in HTML environments, Performance.now() was introduced to allow to measure time (similar to the Stopwatch in .NET) instead of simply using Date.now().

However, to correctly calculate the remaining timeout in the Try...() methods, we should use a time measurement that doesn't include the time which the system spent in standby/hibernation, to match the behavior of Wait() APIs. Of course, it would be nice if .NET Core provided an API to measure time without including the standby/hibernation time, but as that is currently not the case, I needed to use P/Invoke to call a native API on Windows (and fall back to Stopwatch on other OSes).

I have now also checked if UWP Apps can access this API, and this seems to work. Nevertheless, I adjusted the code in TimeUtils to check if the API can actually be called successfully, and if not, fall back to the Stopwatch, so there shouldn't be any problems by using this API.

I have not yet checked how other OSes behave for measuring time when the system goes to standby/hibernation.

DonaldAirey commented 5 years ago

OK. Thanks. I'm good. Very good library. Well done!

kpreisser commented 5 years ago

Regarding the QueryUnbiasedInterruptTime() API, see also the following implementation of time measurement in .NET Core, which is also used by SemaphoreSlim.WaitAsync() and Task.Delay():

        private static int TickCount
        {
            get
            {
                // We need to keep our notion of time synchronized with the calls to SleepEx that drive
                // the underlying native timer.  In Win8, SleepEx does not count the time the machine spends
                // sleeping/hibernating.  Environment.TickCount (GetTickCount) *does* count that time,
                // so we will get out of sync with SleepEx if we use that method.
                //
                // So, on Win8, we use QueryUnbiasedInterruptTime instead; this does not count time spent
                // in sleep/hibernate mode.
                if (Environment.IsWindows8OrAbove)
                {
                    ulong time100ns;

                    bool result = Interop.Kernel32.QueryUnbiasedInterruptTime(out time100ns);
                    if (!result)
                        Marshal.ThrowExceptionForHR(Marshal.GetLastWin32Error());

                    // convert to 100ns to milliseconds, and truncate to 32 bits.
                    return (int)(uint)(time100ns / 10000);
                }
                else
                {
                    return Environment.TickCount;
                }
            }
        }

A difference is that they use QueryUnbiasedInterruptTime() only on Windows 8 and higher (not in Windows 7), and they fall back to Environment.TickCount. I also used Environment.TickCount instead of Stopwatch previously, but didn't like it that much because it is only a 32-bit value (and therefore will overflow after 49.7 days).