Skip to content

Conversation

@SimaTian
Copy link
Member

and making it into an abstract overrridden function instead

Context

While going through the review for Eric's performance PRs I've had some difficulties in tracking to code flow due to the delegate usage. This is a PR to open a discussion about a possibility of replacing some of the delegates with a combination of an abstract function + override.
The downside is an introduction of a variable to store the factory which is otherwise used from a closure. Refactoring this factory away collides with nodeInProc/nodeOutOfProc separation and could result in code duplication.
If there is a better way to pass the factory around, I'm open to suggestions.

Changes Made

replaced internal delegate void NodeContextCreatedDelegate(NodeContext context);
with
protected abstract void CreateNode(NodeContext context);
so that I keep the idea of NodeProviderOutOfProc & NodeProviderOutOfProcTaskHost can use the same structure while removing the delegate since tracking it add additional level of indirection to the code flow.

Testing

If current tests still work then I will take that as a sign that I didn't break anything.

Notes

A quick PR to open a discussion about some of the delegates we have.
There is at least one similar delegate in place I would like to remove as well if we find this approach reasonable.

@SimaTian SimaTian closed this Apr 28, 2025
@ViktorHofer ViktorHofer deleted the NodeProvider_GetNodes_refactor_delegates branch December 1, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants