Waiting for TaskCompletionSource blocks other requests

[andreasjhkarlsson]

When waiting for a TaskCompletionSource in an async request handler it seems that all other requests are blocked until the TaskCompletionSource is completed.

Reproducer (tested with SelfHost):

public class SampleModule : Nancy.NancyModule
{
    public SampleModule()
    {
        // Should return after 10 seconds
        Get("/threader", async (p)=>
        {
                
            var resultSource = new TaskCompletionSource<DateTime>();

            var thread = new Thread(
                () =>
                {
                    Thread.Sleep(10000);
                    resultSource.SetResult(DateTime.Now);
                }
            );
            thread.Start();

            var finished = await resultSource.Task;

            return string.Format("Thread finished at: {0}", finished);
        });


        // Should return right away
        Get("/echo", (p) =>
        {
            return "Echo?";
        });
    }
}

If you navigate to /threader in a browser tab and then open another tab and navigate to /echo, /echo won't respond until /threader completes also.

My guess is that it's because the wrapped Task never transitions to the 'Running' state, see: http://stackoverflow.com/questions/14836995/is-there-a-way-of-setting-a-task-driven-via-taskcompletionsource-to-status-runn

Tested with 2.0.0-clinteastwood, apologies if it's fixed in head.

[damianh]

Does this also happen with OWIN host (HttpListener / WebListener)?

[andreasjhkarlsson]

No, I just tried with a OwinHost and it does not seem to block other requests.

In fact, after some more testing it seems that all async requests blocks other requests when using Nancy.Hosting.Self. For example:

public class SampleModule : Nancy.NancyModule
{
    public SampleModule()
    {
        // Should return after 10 seconds
        Get("/threader", async (p, ct) =>
        {
            await Task.Delay(10000);

            return string.Format("Thread finished at: {0}", DateTime.Now);
        });

        // Should return right away
        Get("/echo", (p) =>
        {
            return "Echo?";
        });
    }
}

So I guess that core issue is that async blocks on Nancy SelfHost.

[damianh]

Thank you for checking.

…

On 9 Mar 2017 9:13 a.m., "Andreas Karlsson" ***@***.***> wrote: No, I just tried with a OwinHost and it does not seem to block other requests. In fact, after some more testing it seems that all async requests blocks other requests when using Nancy.Hosting.Self. For example: public class SampleModule : Nancy.NancyModule { public SampleModule() { // Should return after 10 seconds Get("/threader", async (p, ct) => { await Task.Delay(10000); return string.Format("Thread finished at: {0}", DateTime.Now); }); // Should return right away Get("/echo", (p) => { return "Echo?"; }); } } So I guess that core issue is that async blocks on Nancy SelfHost. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2714 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgXD8Pd_ZTaYtQw9SRzDUctaPGVJiZks5rj7TCgaJpZM4MWmO9> .

[khellang]

I think this might be because we're awaiting the request processing here. See this comment on the PR that introduced it. This is a pretty serious bug.

[xt0rted]

@khellang want me to send in a pr reverting that one line?

[khellang]

I think we should get #2697 in first 😄

[davidallyoung]

I just got #2697 fixed from my git gaffe, and we should be able to have this resolved soon:tm: .

[khellang]

@andreasjhkarlsson Would you be able to pull the latest NuGet package from the MyGet feed and see if you can reproduce this now?

[andreasjhkarlsson]

2.0.0-Pre1846 still shows the same issue. Full reproducer solution here: https://minfil.org/l2sex6b4bd/reproducer.zip

[thecodejunkie]

ping @khellang @damianh

[damianh]

Nancy/src/Nancy.Hosting.Self/NancyHost.cs

Lines 127 to 142 in ceea695

	Task.Factory.StartNew(async () =>
	{
	try
	{
	while(!this.stop)
	{
	HttpListenerContext context = await this.listener.GetContextAsync().ConfigureAwait(false);
	await this.Process(context).ConfigureAwait(false);
	}
	}
	catch(Exception ex)
	{
	this.configuration.UnhandledExceptionCallback.Invoke(ex);
	throw;
	}
	});

^This looks fairly broken. It is handling one request at a time. It's not related to the TaskCompletionSource in OP's report per se.

https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Host.HttpListener/OwinHttpListener.cs#L201-L202
^ Here you can see the MS Owin impl spawns a task for each request but it also does reference counting, "pump limits" etc.

I don't think I've ever used the Nancy.Hosting.Self; I've always used the MS Owin HttpListener host for my self hosting needs. (There is also OwinWebListenerHost).

@NancyFx/most-valued-minions My opinion is to kill Nancy.Hosting.Self package. If it stays it needs a rewrite.

@andreasjhkarlsson In the mean time, use the MSOWIN host. V4 is on the way, so there is some legs left in it.

There is a v4 of owin http listener https://github.com/aspnet/AspNetKatana/milestones

[grumpydev]

This must have been broken fairlyish recently, it never used to only process one request at a time, but last time I looked at the code was probably pre-asyncawait updates.

[xt0rted]

This is from #2548 which is what I had asked about reverting in #2714 (comment)

[davidallyoung]

We actually changed Self Host earlier this year with this PR to resolve it only processing one request at a time with #2697. I might be missing something here though :D

[cemremengu]

Kestrel is the way to go

…

On Sep 18, 2017 01:30, "David Young" ***@***.***> wrote: We actually changed Self Host earlier this year with this PR to resolve it only processing one request at a time with #2697 <#2697>. I might be missing something here though :D — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2714 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPNXx99hmxl8ngbMS92X2tuSisFtESAks5sjZ1ogaJpZM4MWmO9> .

[damianh]

Kestrel is nice, but the problem for use is that its public API is based on AspNet's HttpContext which is unfortunate.

[khellang]

@damianh https://github.com/khellang/KestrelPureOwin 😉

[damianh]

See the dependency graph? Not pure unfortunately.

[khellang]

That's nothing. Most of it isn't even used (the HTTP abstractions etc.). I bet you could trim some of the assemblies and it'd still run. And if you're targeting .NET Core 2.0, you already have it all on disk (it's not part of publish)

[jchannon]

Depends how you publish ie standalone app

…

On 25 September 2017 at 22:39, Kristian Hellang ***@***.***> wrote: That's nothing. Most of it isn't even used (the HTTP abstractions etc.). I bet you could trim some of the assemblies and it'd still run. And if you're targeting .NET Core 2.0, you already have it all on disk (it's not part of publish) — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#2714 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGapu8OWpZ50tWYiQJGlqCy4a7q8WIXks5smB2CgaJpZM4MWmO9> .

[dpavsrtrl]

What's the path to fix this? Self-hosting is useful exactly because it has no dependencies.

[shmutalov]

Any news?

[bespokebob]

This has already been fixed in #2697

[shmutalov]

Yep, just tested with latest builds. When it will be published to nuget.org?

[bespokebob]

According to #2872, sometime this week or next.

[nkosi23]

Has this bug fix been released on NuGet? I mean for 1.x. Just to know the state/limitations of the builds I'm running, I came across this ticket randomly.

[nkosi23]

Apparently not since the NancyHost options introduced in the commit are not available.

It would be really nice to have a snap 1.X release for Nancy.SelfHost since this is a major bug. The fix is already merged so hopefully releasing should be easy enough.

[nkosi23]

Pinging @thecodejunkie and @khellang

[nkosi23]

Just read in #2833 that apparently using the OWIN Self Host has been recommended "for a long time". Unfortunately I haven't seen this recommendation anywhere in the documentation for self hosting. We will try to switch to this SelfHost then, hopefully this will not be disruptive.