Skip to content

Avoid stack overflows in CompositeEndpointDataSource #44392

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

Closed
wants to merge 1 commit into from

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Oct 6, 2022

Avoid stack overflows in CompositeEndpointDataSource

This stack overflow is a little different than the stack overflow originally reported by a customer in #44085 when integrating YARP. However, it is a regression that could potentially be hit by non-YARP EndpointDataSource implementations.

I accidently hit this stack overflow when preparing an Ignite demo. And while there is a workaround to implement the custom EndpointDataSource more correctly by ensuring the custom GetChangeToken() implementation doesn't return the old change token while cancelling it, this did not cause a stack overflow or really any other problem in .NET 6, so this is a regression.

Fixes #44085

Customer Impact

EndpointDataSource is an advanced API, and defining a custom implementation that fires change tokens is rarer still. See https://grep.app/search?current=3&q=%3A%20EndpointDataSource for example usages.

Examining grep.app shows a few custom implementations that do fire a token, but the ones I've seen so far do so correctly in a way that will not trigger a stack overflow. All of the following examples replace the token before cancelling which works around this issue. I have not found any counterexamples yet.

Regression?

  • Yes
  • No

And this is where I double checked and realized this isn't a regression lol. 😆 I still think this is a good change, but it's not worth servicing. I'll retarget to main.

Risk

  • High
  • Medium
  • Low

[Justify the selection above]

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

- Dispatch CompositeEndpointDataSource.HandleChange to the ThreadPool
@halter73 halter73 added the Servicing-consider Shiproom approval is required for the issue label Oct 6, 2022
@halter73 halter73 requested a review from javiercn as a code owner October 6, 2022 00:33
@ghost ghost added the area-runtime label Oct 6, 2022
@ghost
Copy link

ghost commented Oct 6, 2022

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@halter73 halter73 closed this Oct 6, 2022
@halter73 halter73 deleted the halter73/44085-reopen branch October 6, 2022 01:07
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-consider Shiproom approval is required for the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants