Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[2.1] Port "Do not expand stacktraces when completion exception is rethrown multiple times"#31680

Merged
danmoseley merged 2 commits intodotnet:release/2.1from
pakrym:pakrym/port-31375-2-1
Sep 6, 2018
Merged

[2.1] Port "Do not expand stacktraces when completion exception is rethrown multiple times"#31680
danmoseley merged 2 commits intodotnet:release/2.1from
pakrym:pakrym/port-31375-2-1

Conversation

@pakrym
Copy link

@pakrym pakrym commented Aug 9, 2018

Ports #31375 and #31685
Fixes #31388

2.1 template

Description

Multiple calls to ReadAsync on the faulted pipe would produce exceptions with ever-increasing stacktraces.

Customer Impact

Customers would see exceptions with suspiciously long stacktraces:

aspnet/KestrelHttpServer#2745
aspnet/SignalR#2672

Regression?

No

Risk

Only the way we use ExceptionDispatchInfo inside pipe changes. No other logical changes.

@pakrym pakrym requested a review from joshfree August 9, 2018 19:10
@joshfree joshfree requested a review from muratg August 9, 2018 19:14
@joshfree joshfree requested a review from halter73 August 9, 2018 19:14
@joshfree
Copy link
Member

joshfree commented Aug 9, 2018

@muratg @halter73 please review this backport to /release/2.1

Copy link
Member

@joshfree joshfree left a comment

Choose a reason for hiding this comment

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

Approved

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I just realized this change creates an undesired behavioral change where if you complete a pipe with multiple exceptions, the last exception is now thrown from the other side instead of the first one.

@halter73
Copy link
Member

halter73 commented Aug 9, 2018

I think we'll have to fix this in master too.

@joshfree joshfree added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 10, 2018
@pakrym
Copy link
Author

pakrym commented Aug 10, 2018

I updated this PR to include behaviour fix from #31685

@karelz karelz added this to the 2.1.x milestone Aug 23, 2018
@karelz
Copy link
Member

karelz commented Aug 23, 2018

@pakrym @muratg what are the next steps here?

@pakrym
Copy link
Author

pakrym commented Aug 23, 2018

This is ready to be merged. #31685 fixed tests failures we were seeing in master.

@pakrym
Copy link
Author

pakrym commented Sep 4, 2018

This PR was updated with a regression fix. I'm going to remove NO MERGE label and merge the PR.

/cc @joshfree

@karelz
Copy link
Member

karelz commented Sep 4, 2018

@pakrym did you get shiproom approval? cc @danmosemsft

@pakrym
Copy link
Author

pakrym commented Sep 4, 2018

I got it for 2.1.4 and then we discovered a regression and postponed merging.

@pakrym
Copy link
Author

pakrym commented Sep 4, 2018

I re-added servicing considered label and would wait for re-approval.

@pakrym
Copy link
Author

pakrym commented Sep 4, 2018

@dotnet-bot test Linux x64 Release Build please

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Sep 4, 2018
@danmoseley
Copy link
Member

OK re-added consider label. Please update template if necessary.

@pakrym pakrym removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 6, 2018
@jamshedd jamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 6, 2018
@jamshedd
Copy link
Member

jamshedd commented Sep 6, 2018

Approved for 2.1.5

@danmoseley danmoseley merged commit ab121ec into dotnet:release/2.1 Sep 6, 2018
@danmoseley danmoseley modified the milestones: 2.1.x, 2.1.5 Sep 13, 2018
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release labels Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants