Fix for #153

[stegus64]

This fixes issue #153 by adding ConfigureAwait(false) to all await points in the library.
It also modifies the unit tests so any future problems like this will cause unit tests to fail.

[stegus64]

Added fix also for #154

[ChTimTsubasa]

Thanks for the awesome mocking test case to recreate deadlock!
Let's separate the date problem from the deadlock fix and try to solve one at a time.

[ChTimTsubasa]

It's probably a good idea to change the behavior. We can change the test case to specify the input as UTC time with a Z.

[stegus64]

Not really sure what you mean by this.

Please note that the problem is not in the test case. I dont think you should change the test case.
The issue is that if your client computer is in a timezone after UTC and you send in a normal datetime (with only a date and no timezone specified), the wrong date will be stored in snowflake.
As I said, this can be fixed simply by removing the call to ToUniversalTime()

It is not easy to write a unit test for this scenario since there really is no good way to change the timezone programmatically. The time zone setting in windows is system-wide - it can not be changed per thread or per process. But you can easily recreate the scenario by manually changing the timezone of your development computer to UTC+1 before running the unit tests.

[stegus64]

I suggest that you modify csharpTypeValToSfTypeVal to actually return a Date only, so regardless of the time part specified and the kind of Datetime you should always return a date only.

This can be done with the following change:

                case DbType.Date:
                    destType = SFDataType.DATE.ToString();
                    if (srcVal.GetType() != typeof(DateTime))
                    {
                        throw new SnowflakeDbException(SFError.INVALID_DATA_CONVERSION, srcVal, 
                            srcVal.GetType().ToString(), DbType.Date.ToString());
                    }
                    else
                    {
                        DateTime dt = ((DateTime)srcVal).Date;
                        var ts = dt.Subtract(UnixEpoch);
                        long millis = (long)(ts.TotalMilliseconds);
                        destVal = millis.ToString();
                    }
                    break;

I also suggest adding the following test case to make sure it works regardless of the kind of datetime used and the timezone the computer is in:

        [Test]
        [TestCase("2100-12-31 23:59:59.9999999", DateTimeKind.Utc)]
        [TestCase("2100-12-31 23:59:59.9999999", DateTimeKind.Local)]
        [TestCase("2100-12-31 23:59:59.9999999", DateTimeKind.Unspecified)]
        [TestCase("2200-01-01 00:00:00.0000000", DateTimeKind.Utc)]
        [TestCase("2200-01-01 00:00:00.0000000", DateTimeKind.Local)]
        [TestCase("2200-01-01 00:00:00.0000000", DateTimeKind.Unspecified)]
        [TestCase("1960-01-01 00:00:00.0000000", DateTimeKind.Unspecified)]
        [TestCase("9999-12-31 23:59:59.9999999", DateTimeKind.Unspecified)]
        [TestCase("1982-01-18 16:20:00.6666666", DateTimeKind.Unspecified)]
        [TestCase("1982-01-18 23:59:59.0000000", DateTimeKind.Unspecified)]
        [TestCase(null, DateTimeKind.Unspecified)]
        public void TestConvertDate(string inputTimeStr, object kind=null)
        {
            if (kind == null)
                kind = 0;
            DateTime inputTime;
            if (inputTimeStr == null)
            {
                inputTime = DateTime.Now;
            }
            else
            {
                inputTime = DateTime.ParseExact(inputTimeStr, "yyyy-MM-dd HH:mm:ss.fffffff", CultureInfo.InvariantCulture);
            }
            var dtExpected = inputTime.Date;
            internalTestConvertDate(dtExpected, DateTime.SpecifyKind(inputTime, (DateTimeKind)kind));
        }

        private void internalTestConvertDate(DateTime dtExpected, DateTime testValue)
        {
            var result = SFDataConverter.csharpTypeValToSfTypeVal(System.Data.DbType.Date, testValue);
            // Convert result to DateTime for easier interpretation
            var unixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            DateTime dtResult = unixEpoch.AddMilliseconds(Int64.Parse(result.Item2));
            Assert.AreEqual(dtExpected, dtResult);
        }

[ChTimTsubasa]

Awesome mock case!

[ChTimTsubasa]

Other than adding ConfigureAwait(false), we may also want to consider the method described in https://devblogs.microsoft.com/premier-developer//one-user-scenario-to-rule-them-all/ to have a custom awaiter. 2 benefits are:

1. We don't need to attach ConfigureAwait(false) to every line of await;
2. For 2nd and 3rd party code that we used but don't have control on, we can avoid having deadlock.

[stegus64]

Other than adding ConfigureAwait(false), we may also want to consider the method described in https://devblogs.microsoft.com/premier-developer//one-user-scenario-to-rule-them-all/ to have a custom awaiter. 2 benefits are:

1. We don't need to attach ConfigureAwait(false) to every line of await;
2. For 2nd and 3rd party code that we used but don't have control on, we can avoid having deadlock.

Interesting idea. I had not seen that article.

But note that you would have to add
await Awaiters.DetachCurrentSyncContext();
To every async method in the library that could possibly be called directly or indirectly from a public method. This might be almost as much work as adding ConfigureAwait() at all await points in the library.

But I agree that it is very good that this will work even for 3rd party code.

In the end I think it is mostly a matter of taste.

[ChTimTsubasa]

My understanding is that we only need to call await Awaiters.DetachCurrentSyncContext(); at the start of each public method or the starting point where we call async methods inside a sync method. Once the customer awaiter swaps in, the following awaits in the same methods or in the called methods won't have deadlock anymore. Otherwise the 3rd party code case won't work.

[stegus64]

My understanding is that we only need to call await Awaiters.DetachCurrentSyncContext(); at the start of each public method or the starting point where we call async methods inside a sync method. Once the customer awaiter swaps in, the following awaits in the same methods or in the called methods won't have deadlock anymore. Otherwise the 3rd party code case won't work.

Yes, this was what i meant.

But it is not always so easy to find which async methods are called by sync methods. It is much easier to just make sure that all async methods have this line at the beginning. Just like it is much easier to add ConfigureAwait() to all await points rather than trying to figure out where it is actually needed.

[ChTimTsubasa]

My understanding is that we only need to call await Awaiters.DetachCurrentSyncContext(); at the start of each public method or the starting point where we call async methods inside a sync method. Once the customer awaiter swaps in, the following awaits in the same methods or in the called methods won't have deadlock anymore. Otherwise the 3rd party code case won't work.

Yes, this was what i meant.

But it is not always so easy to find which async methods are called by sync methods. It is much easier to just make sure that all async methods have this line at the beginning. Just like it is much easier to add ConfigureAwait() to all await points rather than trying to figure out where it is actually needed.

I see what you meant. Would the testcase you added failed if there is a 3rd party code that we used does not follow the ConfigureAwait(false) style? If yes then I would agree with the ConfigureAwait(false) solution

[stegus64]

I see what you meant. Would the testcase you added failed if there is a 3rd party code that we used does not follow the ConfigureAwait(false) style? If yes then I would agree with the ConfigureAwait(false) solution

Yes, the testcase should fail even if the problem is in 3rd party code. I think it would fail for any situation that would cause an async deadlock.

[ChTimTsubasa]

@stegus64 Can you remove the fix to #154 in this PR and open another PR for that? We need to make an announcement before hand since that would be an behavior change and let's don't delay this one.

I will submit a testcase PR of your fix for #153 and if that passes we can check in your change.

[stegus64]

This is actually the first time I use a fork and pull requests with a public github project.
I originally tried to create a separate pull request, but I did something wrong so it ended up on the same pr.

Do you have any tips for how I should do this?

[stegus64]

I have now tried to separate the fix for #154 into its own pr. I hope it is correct.

[ChTimTsubasa]

Once tests for #157 clears, I would commit this PR.

[ChTimTsubasa]

@stegus64 Committed to prep-1.0.13 and will be release through 1.0.13 next week. Thanks for your contribution!