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

Fixed a lot of bugs in the async pipeline implementation. #888

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@matthid

matthid commented Feb 5, 2014

Additionally while fixing the bugs:

Implemented dynamic module loading support.
Set the current context to null after finishing the work.
Do not reuse a HttpApplication instance when a inner module has thrown an exception.

@desdesdes

This comment has been minimized.

Contributor

desdesdes commented Mar 10, 2014

Can you squash the commits together?

[API] Fixed a lot of bugs in the async pipeline implementation.
Implemented dynamic module loading support.
Set the current context to null after finishing the work.
Do not reuse a HttpApplication instance when a inner module has thrown an exception.
@matthid

This comment has been minimized.

matthid commented Mar 14, 2014

Yes, it's done.

@monoadmin

This comment has been minimized.

monoadmin commented Apr 19, 2014

Need maintainer approval to build this pull request.

@martinjt

This comment has been minimized.

Contributor

martinjt commented Apr 24, 2014

Who needs to approve this?

I saw @migueldeicaza commenting on the commits in the original pull, but this doesn't look to have been looked at yet...

I can run some testing on this locally if that will help... I'm trying to get the boilerplate MVC5 app up and running, and this is the first bug I've come to (RegisterModule method due to Owin).

@alexrp

This comment has been minimized.

Member

alexrp commented Apr 24, 2014

You can ignore that comment from @monoadmin - that's just the experimental pull request CI infrastructure.

@martinjt

This comment has been minimized.

Contributor

martinjt commented Apr 24, 2014

@alexrp thanks, I've been following the spam from that change!

Maybe "Approve" wasn't the best word... who can merge this? I'm assuming that someone "owns" this area of code and needs to approve the change before it can be merged?

@martinjt

This comment has been minimized.

Contributor

martinjt commented Apr 25, 2014

Is this some kind of blackhole? is there noone who can review this so it can be merged? I've applied this to a local instance and it appears to be working ok for the moment.

@kumpera

This comment has been minimized.

Member

kumpera commented Apr 25, 2014

@grendello Please review & merge the above changes.

@migueldeicaza

This comment has been minimized.

Member

migueldeicaza commented May 2, 2014

Please do not merge until you talk to me. This is a delicate part of our pipeline, and I want to do a side-by-side review with someone.

//
// If we catch an error, queue this error
//
void ProcessError (Exception e)
{
// TODO: add logging here (this would be incredible usefull here)

This comment has been minimized.

@migueldeicaza

migueldeicaza May 2, 2014

Member

This todo probably needs to be implemented.

That said: I am surprised that we are not using some sort of aggregate exception to log these errors.

This comment has been minimized.

@matthid

matthid May 3, 2014

Note that this was a recommendation from my side, this means that I found a lot of bugs by adding some sort of logging here.
Basically what needs to be done, if it should be done properly, is to add a custimizable TraceSource so users can add the logging they need into their applications (as it is possible in the .NET framework). However this was out of scope for my use case.

yield return stop_processing;
else if (stop_processing)
yield return true;
yield return false; // let the handler finish it's work

This comment has been minimized.

@migueldeicaza

migueldeicaza May 2, 2014

Member

This looks like a change in the semantics, we need an explanation for this change.

This comment has been minimized.

@matthid

matthid May 3, 2014

Sure.
I think this was a race condition bug.
Assume the following: current_ai.begin immediatly sets the stop_processing flag and than does a lot of cleanup work. When the now parallel running hook manages to set the stop_processing flag before the "yield return stop_processing" is called we now would have two async handlers running at the same time. This can result in anything (almost impossible to debug/test), because this API is not designed to have two handlers running at the same time.

With this version I first let the handler finish (=> yielding false here). And when the handler returns, the code will resume from this point and check the stop_processing flag in line 1091. This should take care of the "expected" behavior.

In fact in the old version in the other case, when the handler was to slow to set the flag immediatly, the following would happen: The handler starts and we return false (which is correct), but after the handler returns we will not check the stop_processing flag (else if!) and execute the next handler, which in fact would than have the semantics described above (running in parallel with our cleanup).

Note especially this change made a lot of wierd behavior go away for me.
Hope this helps.

This comment has been minimized.

@knocte

knocte May 3, 2014

Contributor

That explanation should be inside a commit message of a separate commit.

This comment has been minimized.

@matthid

matthid May 3, 2014

Feel free to fix it, note that after working on this area I understand why miguel wants a side by side review for this change. The pipeline is incredible error prone. However I can write a lot of nonsense in commit messages, especially in this area. So I think it is even better that you have to look into the source code and really understand what's going on.

One more sidenote: I was really impressed how you can emulate async with the yield return statement. I have never seen something like this before and I was blown away (really an amazing idea!)

HttpApplicationFactory.Recycle (app);
if (recycleApp/* && ihh.IsReusable*/)

This comment has been minimized.

@migueldeicaza

migueldeicaza May 2, 2014

Member

I rather not get dead code into the codebase.

This comment has been minimized.

@matthid

matthid May 3, 2014

I was not really sure about this change, I think it "should" be uncommented. However I also think this would/could break some things. That being said I added it as a comment so somebody can comment/fix this.

@migueldeicaza

This comment has been minimized.

Member

migueldeicaza commented May 2, 2014

Do we have a test suite that shows what these changes will enable?

@matthid

This comment has been minimized.

matthid commented May 3, 2014

I have no test suite, but this is a collection of bugfixes I did in order to make MVC5 run stable for me.

@grendello

This comment has been minimized.

Member

grendello commented Jun 17, 2014

I'll try to review the changes tomorrow and offer my pov

@stanislavromanov

This comment has been minimized.

stanislavromanov commented Jun 22, 2014

👍

@pgrm

This comment has been minimized.

pgrm commented Jul 15, 2014

Hi, any updates on this PR?

@stanislavromanov

This comment has been minimized.

stanislavromanov commented Jul 15, 2014

@pgrm

I think we will just have to wait until K runtime arrives with VS2014 :)

@nmschulte

This comment has been minimized.

nmschulte commented Aug 29, 2014

What do I need to do to get this pulled in? I don't think waiting for the K runtime is actually going to solve any real issues folks might have without this PR. I know I would love to have it.

It looks like the logging functionality mentioned is necessary? The bit about the commented (dead) code; that needs understood better?

@monoadmin

This comment has been minimized.

monoadmin commented Aug 29, 2014

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@kumpera kumpera force-pushed the mono:master branch to 727f8fe Oct 15, 2014

@revprez

This comment has been minimized.

revprez commented Oct 26, 2014

@martinjt
@nmschulte

Sorry for beating a dead horse, but was wondering if August was the end of the line for this PR, or if the underlying issues were addressed elsewhere. As of 3.8.0 (September 2014), the HttpApplicaiton.RegisterModule matter is still an issue. Know you guys aren't the submitter or the reviewers, but figured you'd either moved on or found some work around.

@martinjt

This comment has been minimized.

Contributor

martinjt commented Oct 26, 2014

Prez

We're tackling this under a number of other Pr's. Subscribe yourself to
the mailing list and you'll see updates there.

I'm not sure the dynamic module helper is on my list, so it may already be
fixed in Master.

Martin
On 26 Oct 2014 02:04, "Prez Cannady" notifications@github.com wrote:

@martinjt https://github.com/martinjt
@nmschulte https://github.com/nmschulte

Sorry for beating a dead horse, but was wondering if August was the end of
the line for this PR, or if the underlying issues were addressed elsewhere.
As of 3.8.0 (September 2014), the HttpApplicaiton.RegisterModule matter is
still an issue.


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

@revprez

This comment has been minimized.

revprez commented Oct 26, 2014

@martinjt

Thanks. Will do.

@djarvis

This comment has been minimized.

djarvis commented Jan 20, 2015

Hi. I have not seen or heard of any updates in quite some time . As of 1/20/2015, there are still some missing methods in System.Web.Routing.RouteCollection:

public bool AppendTrailingSlash { get; set; }
public bool LowercaseUrls { get; set; }

as well as System.Web.HttpApplication.RegisterModule().

Any searches out there asking for MVC5 compatibility point me to this pull #888. What I do not know is if there are any more recent pull requests with these additions in order to make an MVC5 application minimally function. Does anyone have any insight to this? Are folks just bagging this effort and turning to ASP.NET vNext for MVC5/MVC6 app compatibility for other platforms?

@martinjt

This comment has been minimized.

Contributor

martinjt commented Jan 20, 2015

The ones you mention are still on me, it should be easier now the
referencesource is here. I've just started a new job so that's sapping a
lot of my time.

To give you a general update, MVC 5, from what I remember, only needs those
2 you mention then, for the most part it works. There are some issues that
I didn't get to the bottom of, but most the solutions I tried were around
90-100% functional.

There is an ongoing task for fixing the async pipeline by reusing the. Net
code, but that is nowhere at the moment I think as I tried and it got big,
fast...

888 will not be merged, however, most of the things in it have now been
developed and merged separately.

Hope that helps.
Martin
On 20 Jan 2015 21:04, "Dan Jarvis" notifications@github.com wrote:

Hi. I have not seen or heard of any updates in quite some time . As of
1/20/2015, there are still some missing methods in
System.Web.Routing.RouteCollection:

public bool AppendTrailingSlash { get; set; }
public bool LowercaseUrls { get; set; }

as well as System.Web.HttpApplication.RegisterModule().

Any searches out there asking for MVC5 compatibility point me to this pull
#888 #888. What I do not know is if
there are any more recent pull requests with these additions in order to
make an MVC5 application minimally function. Does anyone have any insight
to this? Are folks just bagging this effort and turning to ASP.NET vNext
for MVC5/MVC6 app compatibility for other platforms?


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

@stanislavromanov

This comment has been minimized.

stanislavromanov commented Jan 21, 2015

Is this really needed now since we will have K runtime with Roslyn in few months?

@pgrm

This comment has been minimized.

pgrm commented Jan 21, 2015

I'll be waiting for ASP.Net 5 and afterwards using probably the following docker image to deploy my applications: https://registry.hub.docker.com/u/microsoft/aspnet/ - and until then I can use what there is so far, it works great

@stanislavromanov

This comment has been minimized.

stanislavromanov commented Jan 21, 2015

@pgrm Yea but MVC5 doesn't work fully with Mono ATM.

@pgrm

This comment has been minimized.

pgrm commented Jan 21, 2015

Yes, for now I'm using only MVC 4 - AFAIK only feature I am missing is the async controller, which would make my application more efficient, but as I don't have too many concurrent users at the moment, I'm fine without it and will later this year just update to MVC 6 (skipping 5)

@martinjt

This comment has been minimized.

Contributor

martinjt commented Jan 21, 2015

This is likely to still be required as CoreCLR is the only framework that will be cross platform. So anyone who is willing to refactor their application around this will be fine, buy you won't beagle to simply deploy a net45 version and run on the "xre" (new name for kre). Probably better to discuss this on the mono list as Miguel can explain it alot better.

@akoeplinger

This comment has been minimized.

Member

akoeplinger commented Jan 21, 2015

@martinjt Mono is a supported target for ASP.NET 5 "vNext", in addition to the new coreclr runtime.

@djarvis

This comment has been minimized.

djarvis commented Jan 21, 2015

@martinjt This is what I was thinking. It's questionable when the CoreCLR for, say, Linux will be released and what packages will be immediately available to pull down. Taken from http://www.asp.net/vnext/overview/aspnet-vnext/aspnet-5-overview we have:

"We will release a cross-platform runtime for Linux and Mac OS X. When released, this runtime will enable you to develop and run .NET apps on Mac and Linux devices. We will work closely with the Mono community on this effort. Until its release, you can use the Mono CLR for cross-platform development."

@lewurm

This comment has been minimized.

Member

lewurm commented Mar 28, 2016

is there still interest to merge this or should it be closed?

@monojenkins

This comment has been minimized.

Contributor

monojenkins commented Mar 28, 2016

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@stanislavromanov

This comment has been minimized.

stanislavromanov commented Mar 29, 2016

@lewing I don't think so. MVC6 is soon to be RC2 already and it runs on Mono without any problems whatsoever.

@lewurm lewurm closed this Mar 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment