Legacy nancy module

[grumpydev]

TL;DR - Find/replace NancyModule with LegacyNancyModule for backwards compatibility.

The "old" NancyModule is now LegacyNancyModule and marked with an ObsoleteAttribute, this can be continued to be used for the immediate future, but the new NancyModule, which has async "only" routing, is the new standard moving forwards. The new NancyModule route definitions are the same as the old NancyModule "async" definitions, but without the pointless boolean on the end, e.g. this:

Get["/", true] = async (_, __) => "foo";

Becomes:

Get["/"] = async (_, __) => "foo";

The initial plan was to use some kind of inheritance to handle the LegacyNancyModule, but this isn't possible as it would hit the same issue of only differing on return type as the original async NancyModule work.

All the demos have been updated to use LegacyNancyModule rather than reworked to use the new one - this should serve as a reminder that we do need to remove it at some point :)

In addition to this work we could also do with Task<dynamic> versions of View, Negotiate etc., although some of those might be tricky as we may hit the "can't differ by return type" issue again so we need to have a think about that.

[thecodejunkie]

Looks good to me! Let's get a second pair of eyes on in and then pull it @NancyFx/most-valued-minions

[khellang]

Nit: Missing ) at the end.

[khellang]

LGTM (modulo one tiny nit)

[grumpydev]

Fixed the typo - also had a think about View etc. and I think we can potentially move all the current things that return Response to be async then add some code to either the conneg, or just before the connect, that does:

var taskResponse = response as Task;
if (taskResponse != null) // simplistic but you get the idea
{
    taskResponse.ConfigureAwait(continueOnCapturedContext: false);
    response = taskResponse.Result;
}

e.g. If View["foo"] was executed from a legacy module it would return Task as the dynamic response, which would automatically get "unwrapped" into ViewResult before going into conneg.

[jchannon]

the new NancyModule, which has async "only" routing, - so even if I return "hello" from a route thats async by default or do I still have to add the async keyword

can it be non async?

[grumpydev]

Yep, previously it would have been horrible because you'd have to return Task, but async handles all that for you so as long as you mark the delegate as async you can do:

return "Foo";

[jchannon]

So I can still return "Foo" without the async keyword so it becomes
non-async?

On 25 January 2016 at 11:42, Steven Robbins [email protected]
wrote:

Yep, previously it would have been horrible because you'd have to return
Task, but async handles all that for you so as long as you mark the
delegate as async you can do:

return "Foo";

—
Reply to this email directly or view it on GitHub
#2199 (comment).

[khellang]

I'm not sure I'd add async just for that, though. Simply because of the insane amount of generated code (see https://gist.github.com/khellang/6e7b3bd197657112714c). A Task.FromResult("Foo") would be better IMO.

[jchannon]

What I'm getting at is I shouldn't mark return "foo" as async as it
serves no purpose making it async

On 25 January 2016 at 11:43, Kristian Hellang [email protected]
wrote:

I'm not sure I'd add async just for that, though. Simply because of the
insane amount of generated code (see
https://gist.github.com/khellang/6e7b3bd197657112714c). A
Task.FromResult("Foo") would be better IMO.

—
Reply to this email directly or view it on GitHub
#2199 (comment).

[grumpydev]

Each to their own, Task.FromResult will work fine, but the generated state machines are heavily optimised for this use case with async/await (which they weren't with Tasks, hence Nancy's original Task helper stuff)

[khellang]

@jchannon Right. So return Task.FromResult("Foo") then...

[grumpydev]

@jchannon : it does serve a purpose, it changes the return type to Task and unwraps it for you, otherwise you have to return a Task yourself, you can't just return a string.

[khellang]

response = taskResponse.Result

@grumpydev That could potentially block, right?

[grumpydev]

Block or deadlock? It could block yes, but that's what the legacy stuff would do anyway, this is just another step towards everything being async under the hood (the response class should be next, but view engine generation etc)

[khellang]

Oh, this is if a route in a legacy module is returning a Task? I guess it might work. We should just make sure it doesn't deadlock 😛

[grumpydev]

Yeah, the configure await is there to hopefully avoid deadlocks, this idea was just to enable us to move things like Response and View to returning Task rather than just T without breaking the legacy stuff .

[jchannon]

But isn't making something async when it isn't async causing unnecessary
overhead in terms of context switches etc?

On 25 January 2016 at 11:52, Steven Robbins [email protected]
wrote:

Yeah, the configure await is there to hopefully avoid deadlocks, this idea
was just to enable us to move things like Response and View to returning
Task rather than just T without breaking the legacy stuff .

—
Reply to this email directly or view it on GitHub
#2199 (comment).

[phillip-haydon]

👍

[grumpydev]

@jchannon : no, that's where the optimisation comes in (previously hand rolled, now using async/await), if a Task is already complete (which it will be when using Task.FromResult or using an async method with no awaits) then it just executes the next code directly.

[jchannon]

OK cool

On 25 January 2016 at 12:12, Steven Robbins [email protected]
wrote:

@jchannon https://github.com/jchannon : no, that's where the
optimisation comes in (previously hand rolled, now using async/await), if a
Task is already complete (which it will be when using Task.FromResult or
using an async method with no awaits) then it just executes the next code
directly.

—
Reply to this email directly or view it on GitHub
#2199 (comment).

[damianh]

taskResponse.Result

I think @khellang mentioned taskRepsonse.GetAwaiter().GetResult(); is better if you don't want AggregateException http://stackoverflow.com/a/17284612