Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added .NET Standard 1.3 and 1.6 support for NotThrowAfter #1050

Merged
merged 2 commits into from May 27, 2019

Conversation

Projects
None yet
4 participants
@davidomid
Copy link
Contributor

commented May 26, 2019

Resolves issue #1049

@davidomid

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

@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?

@dennisdoomen

This comment has been minimized.

Copy link
Member

commented May 26, 2019

@dennisdoomen the test failures appear to be due to the usual flakiness of tests involving timing, and usually pass.

It's something we should ultimately fix using the ITimer abstraction introduced by @lg2de (and now reworked in #1048)

@dennisdoomen dennisdoomen requested a review from jnyrup May 26, 2019

@jnyrup

jnyrup approved these changes May 26, 2019

@@ -167,14 +164,13 @@ public void NotThrowAfter(TimeSpan waitTime, TimeSpan pollInterval, string becau
}

invocationEndTime = watch.Elapsed;
Thread.Sleep(pollInterval);
Task.Delay(pollInterval).GetAwaiter().GetResult();

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 26, 2019

Collaborator

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

This comment has been minimized.

Copy link
@davidomid

davidomid May 26, 2019

Author Contributor

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 :)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 27, 2019

Member

Wondering the same myself...

This comment has been minimized.

Copy link
@davidomid

davidomid May 27, 2019

Author Contributor

@dennisdoomen @jnyrup

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?

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 27, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@davidomid

davidomid May 27, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 27, 2019

Member

I'm fine with that.

@lg2de

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

It's something we should ultimately fix using the ITimer abstraction introduced by @lg2de (and now reworked in #1048)

Yes, please! Everything using Task.Delay or Thread.Sleep or similar should be routed through ITimer.
This should also cover and explain how to wait correctly in all the different contexts. Until today I did not understand why Task.Delay(pollInterval).GetAwaiter().GetResult() should be better (or not?) than Thread.Sleep(pollInterval). For me, everything which is a blocking call without timeout may cause dead-lock.

@davidomid

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

It's something we should ultimately fix using the ITimer abstraction introduced by @lg2de (and now reworked in #1048)

Yes, please! Everything using Task.Delay or Thread.Sleep or similar should be routed through ITimer.
This should also cover and explain how to wait correctly in all the different contexts. Until today I did not understand why Task.Delay(pollInterval).GetAwaiter().GetResult() should be better (or not?) than Thread.Sleep(pollInterval). For me, everything which is a blocking call without timeout may cause dead-lock.

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.

@lg2de

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@davidomid please create a new issue first to have a detailed discussion.
I will collect my experiences here as well.

@dennisdoomen dennisdoomen merged commit b829d3e into fluentassertions:master May 27, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.