search

aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

async all the things #206

Closed davidpeden3 closed 7 years ago

dnfclas commented 7 years ago

Hi @davidpeden3, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

davidfowl commented 7 years ago

@davidfowl let's not, this is going to make the perf even worse.

Let's see some measurements before making those claims. Caching non generic tasks are super easy. As soon as we get rules that read the request body or write to the response body then we need an async option.

Also, it's a pretty big breaking change for anyone that's already implemented rules.

We can make breaking changes in 2.0 and my guess is that we wrote most of the rules since this middleware hasn't been around that wrong.

davidpeden3 commented 7 years ago

@Tratcher is this what you were suggesting?

https://gist.github.com/davidpeden3/1ef73ea305fe262da5aab60150219131

davidpeden3 commented 7 years ago

@muratg I thought this one was done. But it was unclear if you guys wanted to merge it in. Can you clarify the status? Thanks.

Tratcher commented 7 years ago

The question of usefulness vs perf impact was never resolved. That's on us to resolve. @davidfowl

davidfowl commented 7 years ago

Super easy to write a micro benchmark for this so lets do that.

/cc @muratg can you assign somebody to do this?

muratg commented 7 years ago

@mikaelm12 could you write a couple micro-benchmarks testing this?

davidpeden3 commented 7 years ago

@mikaelm12 can you post your results? thanks

mikaelm12 commented 7 years ago

@davidpeden3 I should have commented when I closed this, sorry. Unfortunately with all the work we've been doing for 2.0. This didn't make it in 🙁
We still very much appreciate your contributions though!

jkotalik commented 7 years ago

@muratg Could we consider this for 2.1? I can try to get this in based on performance results.

JunTaoLuo commented 7 years ago

Changing the IRule interface, wouldn't that constitute a breaking change?

jkotalik commented 7 years ago

@JunTaoLuo is right. We would either need to add a second interface (IAsyncRule) or something else that is hacky.