This repository has been archived by the owner on Dec 19, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 307
Pool DefaultHttpContext [Design] #355
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@halter73 He tried that first, see #355 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.