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.

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

api: When forwarding from Listener onAddresses to Listener2 continue to use onResult #11666

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

kannanjgithub
Copy link
Contributor

@kannanjgithub kannanjgithub commented Nov 5, 2024

When forwarding from Listener onAddresses to Listener2 continue to use onResult and not onResult2 because the latter requires to be called from within synchronization context and it breaks existing code that didn't need to do so when using the old Listener interface.

…ization context to call it from inside of the synchronization context.

Fixes grpc#11662.
Collections.singletonList(new EquivalentAddressGroup(address))))
.setAttributes(Attributes.EMPTY)
.build());
args.getSynchronizationContext().execute(() ->
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -58,7 +60,8 @@ private void resolve() {
List<EquivalentAddressGroup> servers = new ArrayList<>(1);
servers.add(new EquivalentAddressGroup(new DomainSocketAddress(authority)));
resolutionResultBuilder.setAddressesOrError(StatusOr.fromValue(servers));
listener.onResult2(resolutionResultBuilder.build());
args.getSynchronizationContext().execute(() ->
Copy link
Member

Choose a reason for hiding this comment

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

This was also fine, because start() and refresh() are called from the synchronization context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@kannanjgithub kannanjgithub changed the title api: Listener2.onResult2 can only be called from within the synchronization context api: Use synchronization context when forwarding from Listener onAddresses to Listener2 onResult2 Nov 5, 2024
@kannanjgithub kannanjgithub changed the title api: Use synchronization context when forwarding from Listener onAddresses to Listener2 onResult2 api: When forwarding from Listener onAddresses to Listener2 continue to use onResult Nov 5, 2024
@kannanjgithub kannanjgithub merged commit dae078c into grpc:master Nov 5, 2024
15 checks passed
@kannanjgithub kannanjgithub deleted the onresult2synccontext branch November 5, 2024 18:22
@ejona86
Copy link
Member

ejona86 commented Nov 12, 2024

Fixes #11662

@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO:backport PR needs to be backported. Removed after backport complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants