Fixing Akka.Remote logging. We don't log the stack trace for known situations. (#2452) #2453
Conversation
| @@ -591,7 +591,7 @@ protected override SupervisorStrategy SupervisorStrategy() | |||
| { | |||
| var causedBy = ia.InnerException == null | |||
| ? "" | |||
| : string.Format("Caused by: [{0}]", ia.InnerException); | |||
| : $"Caused by: [{ia.InnerException}]"; | |||
There was a problem hiding this comment.
It seems that the concept of the known issue was here before. If there is inner exception, we print the message and the stack trace. No changes here.
| { | ||
| // Prepare a Task and pass its completion source to the manager | ||
| var statusPromise = new TaskCompletionSource<AssociationHandle>(); | ||
|
|
||
| manager.Tell(new AssociateUnderlyingRefuseUid(SchemeAugmenter.RemoveScheme(remoteAddress), statusPromise, refuseUid)); | ||
|
|
||
| return statusPromise.Task.CastTask<AssociationHandle, AkkaProtocolHandle>(); | ||
| return (AkkaProtocolHandle)await statusPromise.Task; |
There was a problem hiding this comment.
The code is simplified here, cause CastTask generated AggregatedException of AggregatedException of.... so on the higher level there was no way to reliably handle exceptions. I believe this should be relatively safe.
| @@ -281,32 +281,36 @@ public HeliosTcpTransport(ActorSystem system, Config config) | |||
| /// <param name="remoteAddress">TBD</param> | |||
| /// <exception cref="HeliosConnectionException">TBD</exception> | |||
| /// <returns>TBD</returns> | |||
| protected override Task<AssociationHandle> AssociateInternal(Address remoteAddress) | |||
| protected override async Task<AssociationHandle> AssociateInternal(Address remoteAddress) | |||
There was a problem hiding this comment.
This is the key change here. I don't think we need to throw some Helios exceptions here, cause no one is waiting them and we are not following the interface - if we know what is happening, we should throw InvalidAssociationException with Human readable message and nothing on the inner exception, if not - like usual - just don't catch it.
| /// <param name="localAddress">TBD</param> | ||
| /// <param name="remoteAddress">TbD</param> | ||
| /// <param name="cause">TBD</param> | ||
| /// <param name="disassociateInfo">TBD</param> | ||
| public InvalidAssociation(Address localAddress, Address remoteAddress, Exception cause = null, DisassociateInfo? disassociateInfo = null) | ||
| : base(string.Format("Invalid address: {0}", remoteAddress), cause) | ||
| public InvalidAssociation(string message, Address localAddress, Address remoteAddress, Exception cause = null, DisassociateInfo? disassociateInfo = null) |
There was a problem hiding this comment.
Hardcoded message removed, parameter added
There was a problem hiding this comment.
Could you please add these as constructor overloads, rather than modifying the original constructors? I doubt that anyone is using these public APIs in their own code but for the sake of consistency, let's not modify the public API if we can avoid it.
There was a problem hiding this comment.
Hah! These are internal classes. Nevermind!
| } | ||
| catch (Exception e) | ||
| { | ||
| return new Status.Failure(e.InnerException); |
There was a problem hiding this comment.
We still get the AggregatedException here because of PipeTo - hence we take the inner exception, which is either InvalidAssociationException or some unhandled exception - in any case we propagate this further.
There was a problem hiding this comment.
This change looks good to me... I think TaskScheduler.Default will still get used here, but even if it doesn't I don't think it will matter given how simple the continuation is (object / exception mapping.)
| PublishAndThrow( | ||
| new EndpointAssociationException(string.Format("Association failed with {0}", | ||
| RemoteAddress)), LogLevel.DebugLevel); | ||
| if (failure.Cause.InnerException == null) |
There was a problem hiding this comment.
In here there is a mapping from InvalidAssociationException to InvalidAssociation exception. This mapping was here before! Again - no inner exception - situation is under control - we know what is happening.
Everything else mapped and propagated with the original exception inside. Btw old implementation was hiding it.
| { | ||
| PublishAndThrow( | ||
| new EndpointAssociationException(string.Format("Association failed with {0}", | ||
| RemoteAddress)), LogLevel.DebugLevel); |
There was a problem hiding this comment.
Here we were hiding unhandled exceptions - not cool!
| return new Handle(handle.Result); | ||
| }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default) | ||
| .PipeTo(Self); | ||
| AssociateAsync().PipeTo(Self); |
There was a problem hiding this comment.
Same story here - simplification in order to get rid of AggregatedExceptions. I am fully following PipeTo guideline here.
| case DisassociateInfo.Shutdown: | ||
| throw new ShutDownAssociation(LocalAddress, RemoteAddress, new InvalidAssociationException("The remote system terminated the association because it is shutting down.")); | ||
| throw new ShutDownAssociation($"The remote system terminated the association because it is shutting down. Shut down address: {RemoteAddress}", LocalAddress, RemoteAddress); |
There was a problem hiding this comment.
Removed unnecessary inner exceptions - no one is waiting for them on the higher level, so we were throwing them here for nothing.
| /// <param name="localAddress">TBD</param> | ||
| /// <param name="remoteAddress">TbD</param> | ||
| /// <param name="cause">TBD</param> | ||
| /// <param name="disassociateInfo">TBD</param> | ||
| public InvalidAssociation(Address localAddress, Address remoteAddress, Exception cause = null, DisassociateInfo? disassociateInfo = null) | ||
| : base(string.Format("Invalid address: {0}", remoteAddress), cause) | ||
| public InvalidAssociation(string message, Address localAddress, Address remoteAddress, Exception cause = null, DisassociateInfo? disassociateInfo = null) |
There was a problem hiding this comment.
Could you please add these as constructor overloads, rather than modifying the original constructors? I doubt that anyone is using these public APIs in their own code but for the sake of consistency, let's not modify the public API if we can avoid it.
| /// <param name="localAddress">TBD</param> | ||
| /// <param name="remoteAddress">TbD</param> | ||
| /// <param name="cause">TBD</param> | ||
| /// <param name="disassociateInfo">TBD</param> | ||
| public InvalidAssociation(Address localAddress, Address remoteAddress, Exception cause = null, DisassociateInfo? disassociateInfo = null) | ||
| : base(string.Format("Invalid address: {0}", remoteAddress), cause) | ||
| public InvalidAssociation(string message, Address localAddress, Address remoteAddress, Exception cause = null, DisassociateInfo? disassociateInfo = null) |
There was a problem hiding this comment.
Hah! These are internal classes. Nevermind!
| } | ||
| catch (Exception e) | ||
| { | ||
| return new Status.Failure(e.InnerException); |
There was a problem hiding this comment.
This change looks good to me... I think TaskScheduler.Default will still get used here, but even if it doesn't I don't think it will matter given how simple the continuation is (object / exception mapping.)
| PublishAndThrow( | ||
| new EndpointAssociationException(string.Format("Association failed with {0}", | ||
| RemoteAddress)), LogLevel.DebugLevel); | ||
| if (failure.Cause.InnerException == null) |
| var handler = (TcpClientHandler) associate.Pipeline.Last(); | ||
| return await handler.StatusFuture; | ||
| } | ||
| catch (AggregateException e) when (e.InnerException is ConnectException) |
There was a problem hiding this comment.
I had no idea you could even do this with when.... that's awesome.
|
Just finished reviewing some of the logs from MNTR specs that kill off lots of nodes, and the logs are much much cleaner. This is excellent work. Thank you! |
The old and new logs are in the issue #2452