Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added operation cancel operation #46330

Merged
merged 29 commits into from
Feb 16, 2023
Merged

Added operation cancel operation #46330

merged 29 commits into from
Feb 16, 2023

Conversation

singh733
Copy link
Contributor

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jan 30, 2023
@ghost
Copy link

ghost commented Jan 30, 2023

Thanks for your PR, @singh733. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Tratcher
Tratcher reviewed Jan 30, 2023
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd want a similar check in each server (Kestrel, HttpSys, IIS):

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Line 699 in 04bb460

catch (Exception ex)

src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs Outdated Show resolved Hide resolved
src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs Outdated Show resolved Hide resolved
@singh733
Copy link
Contributor Author

singh733 commented Feb 1, 2023 •
edited
Loading

We'd want a similar check in each server (Kestrel, HttpSys, IIS):

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Line 699 in 04bb460

catch (Exception ex)

@Tratcher I have done the changes for the Kestrel server. I am not able to find HttpProtocol file in IIS and Httpsys. Would you please let me know in which file I need to do change in IIS and Httpsys.

mgravell
mgravell reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to test this scenario? I can't see any tests currently

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs Outdated Show resolved Hide resolved
src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs Outdated Show resolved Hide resolved
src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs Outdated Show resolved Hide resolved
@Tratcher
Copy link
Member

Tratcher commented Feb 1, 2023

We'd want a similar check in each server (Kestrel, HttpSys, IIS):

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Line 699 in 04bb460

catch (Exception ex)

@Tratcher Chris Ross FTE I have done the changes for the Kestrel server. I am not able to find HttpProtocol file in IIS and Httpsys. Would you please let me know in which file I need to do change in IIS and Httpsys.

aspnetcore/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs

Line 55 in 4df5658

catch (Exception ex)

aspnetcore/src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs

Line 44 in 4df5658

catch (Exception ex)

Tratcher
Tratcher reviewed Feb 6, 2023
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update HttpSys as well

src/Http/Http.Abstractions/src/StatusCodes.cs Show resolved Hide resolved
src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs Outdated Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs Outdated Show resolved Hide resolved
@Tratcher Tratcher mentioned this pull request Feb 7, 2023
Open
@singh733
Copy link
Contributor Author

singh733 commented Feb 8, 2023

@Tratcher I have resolved comments. Please let me know if any code change required. Tomorrow I will jump on unit tests

Tratcher
Tratcher reviewed Feb 8, 2023
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far so good.

src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs Outdated Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContext.cs Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs Outdated Show resolved Hide resolved
src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs Outdated Show resolved Hide resolved
Tratcher
Tratcher reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for tests

src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs Outdated Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs Outdated Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs Show resolved Hide resolved
Tratcher
Tratcher reviewed Feb 13, 2023
View reviewed changes
src/Http/Http.Results/test/ResultsTests.cs Outdated Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs Outdated Show resolved Hide resolved
src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs Outdated Show resolved Hide resolved
src/Http/Http.Results/test/ResultsTests.cs Outdated Show resolved Hide resolved
src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs Outdated Show resolved Hide resolved
Tratcher
Tratcher approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I know that was more involved than you expected.

@Tratcher Tratcher enabled auto-merge (squash) February 16, 2023 16:54
@Tratcher Tratcher added this to the 8.0-preview2 milestone Feb 16, 2023
@Tratcher Tratcher merged commit 9068fcf into dotnet:main Feb 16, 2023
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@goenning goenning mentioned this pull request Jul 25, 2023
Open
@cremor cremor mentioned this pull request Aug 4, 2023
Open
@vishweshbankwar vishweshbankwar mentioned this pull request Oct 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgravell mgravell mgravell left review comments

@halter73 halter73 halter73 left review comments

@martincostello martincostello martincostello left review comments

@campersau campersau campersau left review comments

@Kahbazi Kahbazi Kahbazi left review comments

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@JamesNK JamesNK Awaiting requested review from JamesNK JamesNK is a code owner

Assignees

@Tratcher Tratcher

Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
8.0-preview2
Development

Successfully merging this pull request may close these issues.

Framework does not handle Request cancellation properly
10 participants
@singh733 @Tratcher @mgravell @halter73 @martincostello @campersau @BrennanConroy @Kahbazi @adityamandaleeka @amcasey