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

Fix remoting logging DefaultAddress race condition #7305

Conversation

Arkatufus
Copy link
Contributor

Fixes #7304

var defaultAddress = system.AsInstanceOf<ExtendedActorSystem>().Provider.DefaultAddress;

This line can throw an NRE if remoting is not up and running yet
ExtendedActorSystem.Provider.DefaultAddress => RemoteActorRefProvider.DefaultAddress => Transport.DefaultAddress <-- Transport property is null

Changes

Make sure that we catch any NRE thrown when Logging tries to access ExtendedActorSystem.Provider.DefaultAddress

Copy link
Contributor Author

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

self review

@@ -189,7 +189,7 @@ public ActorPath RootPath
public Deployer Deployer { get; protected set; }

/// <inheritdoc/>
public Address DefaultAddress { get { return Transport.DefaultAddress; } }
public Address DefaultAddress { get { return Transport?.DefaultAddress; } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure we return null instead of throwing an NRE if Transport is still null.

catch // can fail if the ActorSystem (remoting) is not completely started yet
{
return a.Path.ToString();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second guard to make sure that, in the event that any exception is thrown, we still return a valid actor path

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) July 25, 2024 23:28
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb disabled auto-merge July 26, 2024 00:03
@Aaronontheweb Aaronontheweb merged commit 3e2d49c into akkadotnet:dev Jul 26, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition between Logging and Remote when creating custom loggers
2 participants