Added .NET Standard 1.3 and 1.6 support for NotThrowAfter #1050
Conversation
|
@dennisdoomen the test failures appear to be due to the usual flakiness of tests involving timing, and usually pass. Is there a way we can trigger a new build? Alternatively I may be able to look into doing something with the ITimer changes made in another branch, to make tests for these areas more reliable? |
It's something we should ultimately fix using the |
|
|
||
| invocationEndTime = watch.Elapsed; | ||
| Thread.Sleep(pollInterval); | ||
| Task.Delay(pollInterval).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
How about copying the behavior from nunit to get the best possible from all worlds?
internal static void BlockingDelay(int milliseconds)
{
#if !PARALLEL
System.Threading.Tasks.Task.Delay(milliseconds).GetAwaiter().GetResult();
#else
Thread.Sleep(milliseconds);
#endif
}where PARALLEL is defined as
<DefineConstants Condition="'$(TargetFramework)' != 'netstandard1.4'
and '$(TargetFramework)' != 'netcoreapp1.1'">$(DefineConstants);PARALLEL;SERIALIZATION</DefineConstants>we could use the !(NETSTANDARD1_3 || NETSTANDARD1_6) instead of PARALLEL
There was a problem hiding this comment.
I find this interesting, because as far as I'm aware there is no benefit to using one or the other, considering both are blocking calls anyway and from my understanding offer similar performance. For me it makes sense to just use Task.Delay.
Do you know of a particular reason to favour Thread.Sleep in some situations instead? I don't doubt that this was done for a good reason for NUnit, I just want to make sure I understand the decision.
Thanks very much :)
There was a problem hiding this comment.
Wondering the same myself...
There was a problem hiding this comment.
I've looked extensively into this and can't find any reason to use Thread.Sleep instead. Unless I'm missing something, are we happy to say just using Task.Delay is fine?
There was a problem hiding this comment.
I'll be the first to admit that I'm no async expert.
I don't know if there are any differences between the two approaches, which are relevant to us.
E.g. whether Task.Delay().GetAwaiter().GetResult() is sync-over-async and could cause a deadlock.
I found three related SO posts, but no indication of whether one should be avoid at all costs.
https://stackoverflow.com/questions/53648014/what-is-going-on-with-task-delay-wait
https://stackoverflow.com/questions/34052381/thread-sleep2500-vs-task-delay2500-wait
https://stackoverflow.com/questions/20082221/when-to-use-task-delay-when-to-use-thread-sleep
To summarize, I'm fine with Task.Delay.
There was a problem hiding this comment.
That top link is actually really interesting. I'll try to benchmark that at some point and figure out if it's a major issue.
I guess in general we should make sure whenever we're using Thread.Sleep or Task.Delay, we treat it less like "I'm guaranteed to wait exactly x milliseconds" and more like "I'm guaranteed to wait at least x milliseconds".
For now though, are we happy to resolve this comment and continue using Task.Delay for now?
Yes, please! Everything using Task.Delay or Thread.Sleep or similar should be routed through |
I'm happy to have a go at this. It might be best to wait for #1048 to the merged first and do it as a separate issue though, unless everyone is happy to have those changes included in the same branch and PR. As for Task.Delay vs Thread.Sleep when blocking, @jnyrup shared a really interesting Stack Overflow link on a comment above which I'll also investigate further: https://stackoverflow.com/questions/53648014/what-is-going-on-with-task-delay-wait Essentially it looks like Thread.Sleep might be more accurate than Task.Delay as Task.Delay uses a Timer internally which has certain overheads. I'll look more into this but I wouldn't say it's going to be a massive issue if we go with one or the other. |
|
@davidomid please create a new issue first to have a detailed discussion. |
Resolves issue #1049