Pool DefaultHttpContext [Design] #355

benaadams · 2015-09-15T09:11:06Z

Resolves aspnet/HttpAbstractions#402 needs PR aspnet/HttpAbstractions#404 done first

dnfclas · 2015-09-15T09:11:11Z

Hi @benaadams, 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;

benaadams · 2015-09-15T09:48:57Z

See aspnet/HttpAbstractions#402 (comment) for details on server run improvements (61.5k rps -> 1.49M rps)

Tratcher · 2015-09-15T14:08:16Z

src/Microsoft.AspNet.Hosting/Internal/HostingEngine.cs

-                    var requestIdentifier = GetRequestIdentifier(httpContext);
-
-                    using (logger.BeginScope("Request Id: {RequestId}", requestIdentifier))
+                    using (var httpContext = contextFactory.CreateHttpContext(features))


The scope of HttpContext is not this deterministic. IHttpResponseFeature has two events, OnStarting and OnCompleted, that may not have fired before the end of this using block. Both of these events may have user state that contains the HttpContext. I'm not sure there is a simple workaround.

Isn't FireOnCompleted/OnCompleted always called by the end of

await application(httpContext);?

e.g. https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs#L226

Else it will never be called which would be a problem?

I think you have your stack backwards. 'Application' in that kestrel code block refers to the async lamda you're in here (LN71-83), so FireOnCompleted runs after your using block.

Ahh... hmm...

K, I think the problem is also the solution :)

Removed using and added httpContext.Response.RegisterForDispose(httpContext);

Will need to change it so Disposing is done after OnCompleted rather than during; but probably should be anyway?

Tratcher · 2015-09-15T14:18:51Z

This is a little dangerous. HttpContext is the primary touchpoint for application APIs and is likely to be referenced in non-linear ways like events, background threads, etc.. Consider SignalR which does much of its work on background threads and could easily get into race conditions with this change. Arguably SignalR needs to guard against referencing the HttpContext after the 'end' of the request, but before the worst that could happen was an ObjectDisposedException. Now recycling the HttpContext underneath them could cause pretty terrible side effects like having the wrong user credentials.

benaadams · 2015-09-15T15:08:41Z

Consider SignalR which does much of its work on background threads and could easily get into race conditions with this change.

Will get back to you on this; I think I can add a safety layer that will throw errors at a cost of one small object per request.

davidfowl · 2015-09-15T17:12:43Z

src/Microsoft.AspNet.Hosting/Internal/HostingEngine.cs

@@ -71,6 +71,8 @@ public virtual IDisposable Start()
                async features =>
                {
                    var httpContext = contextFactory.CreateHttpContext(features);
+                    httpContext.Response.RegisterForDispose(httpContext);


That is pretty weird...

I think its the weirdest code I've written...

More sensibly it should return Task < IDisposable > rather than Task; but then' I'd have to add a PR to Kestrel as well... Ok will do that?

There's still and ordering problem. If this registers first it will run first. I think you'd have to register after await application to ensure you'd be last.

Going to return IDisposable from the closure; HttpContext has always been IDisposable, just never Disposed so will be just closing that loop.

Needs aspnet/Hosting#355

halter73 · 2015-09-17T01:14:57Z

src/Microsoft.AspNet.Hosting/Internal/HostingEngine.cs

@@ -79,6 +80,8 @@ public virtual IDisposable Start()
                        contextAccessor.HttpContext = httpContext;
                        await application(httpContext);
                    }
+
+                    return httpContext;


Is this so httpContext gets disposed? What are we waiting for?

I would just wrap the call to await application(httpContext) in a using statement. Once the Task returned from the application is completed, you should be safe to dispose the httpContext.

@halter73 He tried that first, see #355 (comment)

Is the HttpContext supposed to be usable in OnCompleted? I think the simplest solution is to not support it. It's not like you would be able to change the response at that point.

I'm not a big fan of returning an IDisposable here, because it's not obvious why it's necessary. It could definitely be a gotcha for anyone implementing or using the IServerFactory interface.

I would even prefer having a "OnTearDown" IHttpResponseFeature that runs after OnCompleted, only because the purpose is more clear.

I'm not recommending any specific solution, just trying to outline what's possible. HttpContext is supported in OnCompleted, but more importantly it's supported in OnStarted which can fire after await application(httpContext) in no-body scenarios.

Is open question on whether the Context is the place to dispose of the features collection: https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http/DefaultHttpContext.cs#L172

Currently it is in Dispose; but the dispose never gets called as its not returned, so the features collection is never disposed?

Have raised related issue #359 as these have all been PRs where its slightly tangential.

davidfowl · 2015-09-19T09:17:03Z

Closing this as discussed with @benaadams

Pool DefaultHttpContext

d3f8d66

dnfclas added the cla-already-signed label Sep 15, 2015

Tratcher reviewed Sep 15, 2015
View reviewed changes

Fix Context disposal

718d821

davidfowl reviewed Sep 15, 2015
View reviewed changes

Return Disposable and use Initalize on context

9dd277a

benaadams added a commit to benaadams/KestrelHttpServer that referenced this pull request Sep 15, 2015

Dispose of IDisposable context

14c6c85

Needs aspnet/Hosting#355

benaadams mentioned this pull request Sep 15, 2015

Dispose of IDisposable context [Design] aspnet/KestrelHttpServer#204

Closed

benaadams changed the title ~~Pool DefaultHttpContext~~ Pool DefaultHttpContext [Design] Sep 15, 2015

halter73 reviewed Sep 17, 2015
View reviewed changes

davidfowl closed this Sep 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pool DefaultHttpContext [Design] #355

Pool DefaultHttpContext [Design] #355

benaadams commented Sep 15, 2015

dnfclas commented Sep 15, 2015

benaadams commented Sep 15, 2015

Tratcher Sep 15, 2015

benaadams Sep 15, 2015

Tratcher Sep 15, 2015

benaadams Sep 15, 2015

benaadams Sep 15, 2015

benaadams Sep 15, 2015

Tratcher commented Sep 15, 2015

benaadams commented Sep 15, 2015

davidfowl Sep 15, 2015

benaadams Sep 15, 2015

Tratcher Sep 15, 2015

benaadams Sep 15, 2015

halter73 Sep 17, 2015

Tratcher Sep 17, 2015

halter73 Sep 17, 2015

Tratcher Sep 17, 2015

benaadams Sep 17, 2015

davidfowl commented Sep 19, 2015

Pool DefaultHttpContext [Design] #355

Pool DefaultHttpContext [Design] #355

Conversation

benaadams commented Sep 15, 2015

dnfclas commented Sep 15, 2015

benaadams commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Sep 15, 2015

benaadams commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Sep 19, 2015