[MLA-1767] Refactor communicator connection exceptions #4935
Conversation
| @@ -64,8 +64,8 @@ public RpcCommunicator(CommunicatorInitParameters communicatorInitParameters) | |||
|
|
|||
| internal static bool CheckCommunicationVersionsAreCompatible( | |||
| string unityCommunicationVersion, | |||
| string pythonApiVersion, | |||
| string pythonLibraryVersion) | |||
There was a problem hiding this comment.
Removed this since it was unused.
| @@ -113,62 +113,75 @@ public UnityRLInitParameters Initialize(CommunicatorInitParameters initParameter | |||
| { | |||
| RlInitializationOutput = academyParameters | |||
| }, | |||
| out input); | |||
|
|
|||
| var pythonPackageVersion = initializationInput.RlInitializationInput.PackageVersion; | |||
There was a problem hiding this comment.
Moved this logic outside of the try. It no longer throws (throw new UnityAgentsException("ICommunicator.Initialize() failed.");) and instead returns false.
| } | ||
| catch | ||
| { | ||
| var exceptionMessage = "The Communicator was unable to connect. Please make sure the External " + |
There was a problem hiding this comment.
Removed this messaging - I'm not sure it was ever useful, since the exception was swallowed by the calling code.
| @@ -456,33 +467,35 @@ UnityInputProto Exchange(UnityOutputProto unityOutput) | |||
| QuitCommandReceived?.Invoke(); | |||
| return message.UnityInput; | |||
| } | |||
| catch (RpcException rpcException) | |||
There was a problem hiding this comment.
Reorganized this to avoid duplicating some of the logic (e.g. m_IsOpen = false). Not sure if we have a preference; it's not mentioned in the style guide.
| @@ -34,9 +35,18 @@ public Guid ChannelId | |||
|
|
|||
| internal void ProcessMessage(byte[] msg) | |||
| { | |||
| using (var incomingMsg = new IncomingMessage(msg)) | |||
| try | |||
There was a problem hiding this comment.
This threw me off when I was working on the Training Analytics side channel. If the SideChannel throws here, we disconnect the trainer. In general, I think we should wrap user code in bubble wrap.
|
You may want to hide whitespace changes when reviewing this |
| Debug.Log($"Unexpected exception when trying to initialize communication: {ex}"); | ||
| } | ||
|
|
||
| if (!initSuccessful) |
There was a problem hiding this comment.
why not an else after the first if (initSuccessful) {}
There was a problem hiding this comment.
It's meant to handle both Communicator.Initialize returning false and throwing. But I think you're right, it's cleaner to handle each separately.
Proposed change(s)
Previously we were
The main change here is to make ICommunicator.Initialize() return a bool. Unexpected exceptions (basically anything but a RpcException with Unavailable status, which is what happens when there's no communicator listening) are logged.
Also, exceptions during SideChannel processing are wrapped in a try/catch, so that a single SideChannel can't bring down the rest of the system.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments