Fix so timers work through system time change

[cmcqueen]

This makes timers operate against CLOCK_MONOTONIC rather than the
default of using system time (CLOCK_REALTIME).

https://bugzilla.xamarin.com/show_bug.cgi?id=3025

[dnfclas]

Hi @cmcqueen, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[monojenkins]

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

[dnfclas]

@cmcqueen, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[akoeplinger]

approve

[evincarofautumn]

#ifdef CLOCK_MONOTONIC only works if CLOCK_MONOTONIC is a #define and not, say, an enum. POSIX specifies #ifdef _POSIX_MONOTONIC_CLOCK to check for availability.

As an aside, this reminds me of a neat trick:

enum {
    …
    FOO = …,
    …
};

#define FOO FOO

Now #ifdef FOO will work, but it’s still an enum. :)

[akoeplinger]

we're using #if defined(CLOCK_MONOTONIC) in mono/profiler/utils.c and mono/utils/mono-time.c

[cmcqueen]

That's true. I would have checked _POSIX_MONOTONIC_CLOCK, but I saw #elif defined(CLOCK_MONOTONIC) in clock_time() in mono/profiler/utils.c and thought I should follow that for consistency. I saw no other instances of checking _POSIX_MONOTONIC_CLOCK in the Mono code.

[cmcqueen]

Note, I also see that mono_os_sem_timedwait() in mono/utils/mono-os-semaphore.h is doing calls to gettimeofday() and time-delta calculations. That would likewise be prone to errors on a system time change. But I don't know enough about Mono to know what platforms or Mono API that would affect. I will just raise the issue and say it looks as though it needs some attention.

[migueldeicaza]

lgtm

[akoeplinger]

I just remembered #3165 and fear we might run into the same problem if we merge this.

[cmcqueen]

This patch uses clock_gettime() but not clock_getres(). Does that make it okay?

[akoeplinger]

@cmcqueen unfortunately not, since the issue is that #ifdef CLOCK_MONOTONIC will be misdetected which applies to this patch as well.

Adding !defined(__APPLE__) to the condition is probably the easiest fix for now.

[cmcqueen]

I've written the code so that if clock_gettime() and pthread_condattr_setclock() fail, then it effectively falls back to using CLOCK_REALTIME (via gettimeofday()). Do you think that will work as intended in the MACOS case? I think it should, unless the clock_gettime() symbol is missing. Am I understanding the issue properly?

[akoeplinger]

@cmcqueen the issue is that the OSX 10.12 SDK defines CLOCK_MONOTONIC/clock_gettime/etc irrespectively of whether those APIs are available on earlier OSX releases (we pass -mmacosx-version-min=10.7).

It means that if we compile with the 10.12 SDK the resulting binary won't run in say 10.11 since it doesn't have those APIs.

[cmcqueen]

Okay, thanks for explaining.

Does the SDK correctly define (or not) _POSIX_MONOTONIC_CLOCK as mentioned in a previous comment, so would using #ifdef _POSIX_MONOTONIC_CLOCK fix the issue?

Otherwise, I guess as you said, I should change the #ifdef to

#ifdef CLOCK_MONOTONIC && !defined(__APPLE__)

or

#ifdef CLOCK_MONOTONIC && !defined(PLATFORM_MACOSX)

I guess that would make all MAC OSX prone to the bug I was originally trying to fix (system time change disrupts timed waits).

[cmcqueen]

Let me know if you want me to make changes. (I'm currently not familiar with the process of modifying/redoing a pull request, but I guess I could figure that out.)

[akoeplinger]

@cmcqueen as far as I remember, it doesn't correctly hide _POSIX_MONOTONIC_CLOCK either.

I think #if defined(CLOCK_MONOTONIC) && !defined(__APPLE__) is the safest bet, you can just add a new commit and push to the timer-fix branch on your fork and it'll automatically show up here.

[cmcqueen]

I've pushed an amended commit.

[akoeplinger]

@cmcqueen seems like this change snuck in, can you please revert it?

[alexrp]

The conditions here can be simplified. We have required CLOCK_MONOTONIC to be present for a while for the runtime to even compile (see mono/mini/mini-posix.c). I don't know of any 'real' POSIX system that doesn't have it these days. So just checking #ifndef __APPLE__ should be sufficient.

[cmcqueen]

I see other cases in the mono code where there is an #if check of CLOCK_MONOTONIC:

• mono_100ns_ticks() in mono/utils/mono-time.c
• clock_time() in profiler/utils.c

If these are simplified to remove the #if check of CLOCK_MONOTONIC, then I'd be happy to simplify this patch to match.

Or, would you like me to simplify this patch anyway?

[alexrp]

It's fine to clean up the code in mono/utils/mono-time.c while you're here. I'm in the middle of cleaning up the relevant profiler code, so no need to bother with that.

[cmcqueen]

What is the current state of this pull request and the bug it was intended to fix? At the point of my last change, I wasn't able to keep modifying it to follow the changes in the related code. Now, what would it take to follow through on this?

[jaymin1328]

issue is still there in latest code

[BrandonLWhite]

This issue is affecting my application in Mono 4.8. I'd really like to see it resolved.

[luhenry]

An equivalent change has been merged with #4446