Saga<T>.RequestTimeout does not respect DateTimeKind

[aluetjen]

When passing a DateTime of kind Utc, the subsctract of DateTime.Now can result in the wrong result.

    protected void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at - DateTime.Now, withState);
    }

Should be:

    protected void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at.ToLocalTime() - DateTime.Now, withState);
    }

[andreasohlund]

Is this on the 2.5 branch? ( the master is using UtcNow?)

https://github.com/NServiceBus/NServiceBus/blob/master/src/core/NServiceBus.Saga/Saga.cs

[aluetjen]

Right. If it is UtcNow, then passing a local time will result in an invalid result. The problem is the DateTime substracts the internal ticks without doing any check or conversion of the kind of datetime.

[aluetjen]

Just noticed Udi's comment here: "Reverting back to DateTime.Now behavior from DateTime.UtcNow as it breaks backwards compatibility." Very true! What we need is a conversion of the passed "at" to whatever DateTimeKind we are retrieving through DateTime.(Utc)Now.

[sagacity]

Is there any update on this?

[sagacity]

Pull request: #44

[bmatson]

Andreas, I'm the one that originally pointed this issue out, and had it changed to use UTCNow. Use of local time to calculate the time span will not work correctly across shifts in time due to going into or out of daylight savings time. All my saga timeouts got messed up this spring when the US entered daylight savings time because I was passing in local time like the DateTime.Now version of the code forces you to do.

The suggested fix for this has the same issue; it is coercing the passed in time (which in my system is now UTC) to local, then calculating the time span.

The only way to make this work reliably is to calculate the time span in a time zone which does not have time shifts every spring and fall (i.e UTC). A better fix to try to address the "backward compatibility" issue would be the following:

protected void RequestTimeout(DateTime at, object withState)
{
RequestTimeout(at.ToUniversalTime() - DateTime.UtcNow, withState);
}

That way if someone is passing in local time (which is a bad idea IMHO), the code will force it to UTC and then calculate the time span correctly since UTC doesn't have daylight savings time shifts.

[andreasohlund]

This should now be ok on 2.5

[aluetjen]

FYI: this is still buggy.

To clarify:

The only way to make this work reliably is to calculate the time span in a time zone which does not have time shift.

Incorrect. At a given point in time, a DateTime value represents single other point in time, independent from whether it is local or UTC.

The only difference between the two is, a different offset from which we measure 100-nanoseconds since 12:00:00 midnight, January 1, 0001. In other words: 12:00:00 midnight, January 1, 0001 was exactly one point in time in New York. And one point in time in London. But they differ.

So this code from the .net framework does not work as you might expect:

    public TimeSpan Subtract(DateTime value)
    { 
        return new TimeSpan(InternalTicks - value.InternalTicks);
    } 

Which means the following does not work as expected either if we pass a local time:

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at - DateTime.UtcNow, withState);
    }

I am executing the code in New York on 12:00:00 midnight, January 1, 0001. And I want a timeout in 1 nanosecond! That will become:

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout( 1 - 12345 (a lot of ticks ahead in London), withState);
    }

Which lead to a timeout in the past instead of in the very near future of 1 nanosecond. Now all we need to do is to safely convert "at" into UTC (right here, right now when the code executes this is a deterministic point in time!).

Or as another simple example: change your timezone to Timbuktu and run:

        DateTime now = DateTime.Now;
        DateTime nowUtc = DateTime.UtcNow;

        Console.WriteLine(now - nowUtc);

The result is != 0.

The implementation of the setters in the TimeoutMessage look a bit scary, too. The ticks are just taken as they are and treated as "You measure from Greenwhich now!", which obviously refers to a different point in time then. Unless we always pass in UTC to the setter, this will give wicked results.

I think this should be done right because of the nature of bugs it can potentially create. It's that phone call at 3am and the beautiful time dependent unit test we love so much to write afterwards.

Just my two cents...

[bmatson]

Nope,

A DateTime value in local time, during the period of time of a DST shift IS NOT unique. 2:30 AM local will happen TWICE on the day of the "fall back" DST shift (at 3:00 AM you shift the clock back to 2:00 AM local).

See the IsAmbiguousTime member of the TimeZoneInfo class....

Since UTC does not do DST changes it needs to be the basis for timeouts of sagas.

Bill

-----Original Message-----
From: aluetjen [mailto:reply@reply.github.com]
Sent: Monday, October 31, 2011 5:22 PM
To: Bill Matson
Subject: Re: [NServiceBus] Saga.RequestTimeout does not respect DateTimeKind (#31)

FYI: this is still buggy.

To clarify:

The only way to make this work reliably is to calculate the time span in a time zone which does not have time shift.

Incorrect. At a given point in time, a DateTime value represents single other point in time, independent from whether it is local or UTC.

The only difference between the two is, a different offset from which we measure 100-nanoseconds since 12:00:00 midnight, January 1, 0001. In other words: 12:00:00 midnight, January 1, 0001 was exactly one point in time in New York. And one point in time in London. But they differ.

So this code from the .net framework does not work as you might expect:

    public TimeSpan Subtract(DateTime value)
    { 
        return new TimeSpan(InternalTicks - value.InternalTicks);
    } 

Which means the following does not work as expected either:

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at - DateTime.UtcNow, withState);
    }

I am executing the code in New York on 12:00:00 midnight, January 1, 0001. And I want a timeout in 1 nanosecond! That will become:

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout( 1 - 12345 (a lot of ticks ahead in London), withState);
    }

Which lead to a timeout in the past instead of in the very near future of 1 nanosecond. Now all we need to do is to safely convert "at" into UTC (right here, right now when the code executes this is a deterministic point in time!).

Or as another simple example: change your timezone to Timbuktu and run:

        DateTime now = DateTime.Now;
        DateTime nowUtc = DateTime.UtcNow;

        Console.WriteLine(now - nowUtc);

The result is != 0.

The implementation of the setters in the TimeoutMessage look a bit scary, too. The ticks are just taken as they are and treated as "You measure from Greenwhich now!", which obviously refers to a different point in time then. Unless we always pass in UTC to the setter, this will give wicked results.

I think this should be done right because of the nature of bugs it can potentially create. It's that phone call at 3am and the beautiful time dependent unit test we love so much to write afterwards.

Just my two cents...

Reply to this email directly or view it on GitHub:
#31 (comment)

[aluetjen]

Yes, you are right. A local time can be ambigious. However, as I said, "At a given point in time" it is not. 2:30 AM in winter refers to a single point in time. But 2:30 AM in summer refers to another. This is why we never persist a local time.

Whatever, my point is that the following code is buggy when you pass a local time. So are the setters in TimeoutMessage.

protected virtual void RequestTimeout(DateTime at, object withState)
{
    RequestTimeout(at - DateTime.UtcNow, withState);
}

[bmatson]

The timeout manager logic uses local times to figure out when to send the message if you look closely at the code. DST shifts mess up the existing logic....

The whole discussion is pretty much moot anyway since starting with 2.6 there is a new RequestTimeoutUTC api in the code that does use UTC times. This issue was an attempt to fix the fundamental problem without changing the API, which Udi pretty much nixed, in preference for leaving the old, buggy code in place in the old API. If you use the old API, your timeouts will be messed up when they span a DST shift, if you use the new one they won't.

Up to you, but the issue is closed AFAIC.

Bill

-----Original Message-----
From: aluetjen [mailto:reply@reply.github.com]
Sent: Tuesday, November 01, 2011 9:47 AM
To: Bill Matson
Subject: Re: [NServiceBus] Saga.RequestTimeout does not respect DateTimeKind (#31)

Yes, you are right. A local time can be ambigious. However, as I said, "At a given point in time" it is not. 2:30 AM in winter refers to a single point in time. But 2:30 AM in summer refers to another. This is why we never persist a local time.

Whatever, my point is that the following code is buggy when you pass a local time. So are the setters in TimeoutMessage.

protected virtual void RequestTimeout(DateTime at, object withState)
{
    RequestTimeout(at - DateTime.UtcNow, withState);
}

Reply to this email directly or view it on GitHub:
#31 (comment)

[aluetjen]

Allow me one last comment on this :) Our point of confusion might basically be that we look at different pieces of source code! Hear me out Sir!

I am looking at the "Master" code branch! Which still has the old "fix" which was rolled back in the 2.5 branch. The code in the master is what disturbs me!

That initial fix should have been exactly what you suggested in your post! And there wouldn't be any problem.....

On the whole story about backwards compatibility. The fix that was rolled back was: Changing "at - DateTime.Now" into "at - DateTime.UtcNow". Which is not valid. The fix YOU requested would have been the right one. There is no problem with backwards compatibility in the API with the correct fix. (Leave alone the whole story what the timeout manager does, my point is only about the API in whatever format we write it onto the wire). Ok, that's it. I shut up. Issue is closed as for as we're concerned.

[bmatson]

The "old" fix works ok as long as you only use UTC times. Under the old code with the DateTime.now in it, in the constructor for the TimeoutMessage it was coercing the passed in time to UTC (line 25). The implementation was basically crazy inconsistent about how it handled the expiration time....

Like you I agree that just looking at the DateKindType of the passed in time (and assuming that DateTimeKind.Unspecified was local for backwards compatibility) was sufficient. Udi didn't see it that way and left the "old" logic in place under the old API, but put in a new API to handle the UTC case. He wasn't comfortable with changing the broken behavior since someone might have implemented some sort of crazy workaround to address the underlying inconsistencies.

In any case the old API that was broke is now deprecated, and the new one that explicitly uses UTC is available....

-----Original Message-----
From: aluetjen [mailto:reply@reply.github.com]
Sent: Tuesday, November 01, 2011 3:58 PM
To: Bill Matson
Subject: Re: [NServiceBus] Saga.RequestTimeout does not respect DateTimeKind (#31)

Allow me one last comment on this :) Our point of confusion might basically be that we look at different pieces of source code! Hear me out Sir!

I am looking at the "Master" code branch! Which still has the old "fix" which was rolled back in the 2.5 branch. The code in the master is what disturbs me!

That initial fix should have been exactly what you suggested in your post! And there wouldn't be any problem.....

On the whole story about backwards compatibility. The fix that was rolled back was: Changing "at - DateTime.Now" into "at - DateTime.UtcNow". Which is not valid. The fix YOU requested would have been the right one. There is no problem with backwards compatibility in the API with the correct fix. (Leave alone the whole story what the timeout manager does, my point is only about the API in whatever format we write it onto the wire). Ok, that's it. I shut up. Issue is closed as for as we're concerned.

Reply to this email directly or view it on GitHub:
#31 (comment)

[bmatson]

This is the same discussion I had with Udi...he said he isn't doing it that way for backwards compatibility....use the UTC api...end of story.

-----Original Message-----
From: aluetjen [mailto:reply@reply.github.com]
Sent: Tuesday, November 01, 2011 4:47 PM
To: Bill Matson
Subject: Re: [NServiceBus] Saga.RequestTimeout does not respect DateTimeKind (#31)

I read you....please read me, too.

Original code we want to stay backwards compatible with:

SAGA NOT TIMEOUTMESSAGE!

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at - DateTime.Now, withState);
    }

SAGA IN MASTER:

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at - DateTime.UtcNow, withState);
    }

This nonsense......

Can you tell me a single valid argument why this shouldn't be:

    protected virtual void RequestTimeout(DateTime at, object withState)
    {
        RequestTimeout(at.ToUniversal() - DateTime.UtcNow, withState);
    }

Please read and think again! And then write a unit test that proves it.....

Sorry for losing it...

Reply to this email directly or view it on GitHub:
#31 (comment)

[aluetjen]

It's a different discussion because the master doesn't have a dedicated UTC API anymore. Instead it has a broken single implementation of Saga.RequestTimeout. So every single written saga using local time will basically break in the current master. If that's acceptable, fair enough. If it's don't clear why, I am happy to provide a unit test.

[bmatson]

Yep, looks like the new api was implemented in the 2.5 branch, but hasn't been moved over to the master. In the master the RequestTimeout with a timespan will work fine, but RequestTimeout with anything other than a UTC time won't. Probably need to look at the new TimeoutManager code to ensure that it properly handles the new API and TimeoutMessage format too.

-----Original Message-----
From: aluetjen [mailto:reply@reply.github.com]
Sent: Wednesday, November 02, 2011 9:35 AM
To: Bill Matson
Subject: Re: [NServiceBus] Saga.RequestTimeout does not respect DateTimeKind (#31)

It's a different discussion because the master doesn't have a dedicated UTC API anymore. Instead it has a broken single implementation of Saga.RequestTimeout. So every single written saga using local time will basically break in the current master. If that's acceptable, fair enough. If it's don't clear why, I am happy to provide a unit test.

Reply to this email directly or view it on GitHub:
#31 (comment)

[aluetjen]

but RequestTimeout with anything other than a UTC time won't

Wow, that took a while....we're getting there. The curently implementation in the saga will misbehave even before the timeout message reaches the manager. Specifically, the saga checks whether the timespan is greather zero.

[bmatson]

Same answer as before....what was done in the 2.5 branch needs to be applied to master.

-----Original Message-----
From: aluetjen [mailto:reply@reply.github.com]
Sent: Wednesday, November 02, 2011 1:07 PM
To: Bill Matson
Subject: Re: [NServiceBus] Saga.RequestTimeout does not respect DateTimeKind (#31)

but RequestTimeout with anything other than a UTC time won't

Wow, that took a while....we're getting there. The curently implementation in the saga will misbehave even before the timeout message reaches the manager. Specifically, the saga checks whether the timespan is greather zero.

Reply to this email directly or view it on GitHub:
#31 (comment)

[aluetjen]

Sure :D Let's grab a bag of popcorn and see where this goes. Thanks for the chat :D