Cleanup model proxy #1345
Merged
Cleanup model proxy #1345
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
- Simplified the model proxy to remove the need for a mutex. - Handle an edge case in the model proxy where if the controller routine returned we don't stop the proxy.
kian99
force-pushed
the
cleanup-model-proxy
branch
from
914ef05 to
9983dee
Compare
kian99
requested review from
alesstimec,
SimoneDutto,
babakks,
ale8k and
pkulik0
ale8k
approved these changes
Sep 4, 2024
| websocket.CloseNoStatusReceived, | ||
| websocket.CloseAbnormalClosure) | ||
| _, unmarshalError := err.(*json.InvalidUnmarshalError) | ||
| return closeError || unmarshalError |
There was a problem hiding this comment.
I don't like this logic, don't do this. Just check if we have a close error, if we do return else check a marshal error. The name is also confusing, as this is an unexpected closure versus unmarshalling.
There was a problem hiding this comment.
Will fix this in a follow-up.
SimoneDutto
approved these changes
Sep 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR recreates #1336 (that PR was coming from my old fork)
This PR modifies the model proxy to improve the error logging and simplify logic in some edge cases.
The reason for this change was that when running JIMM in a test setup there were constant debug logs about "error reading from client" or "error reading from controller" which were completely expected, every time the client closes the websocket our routines waiting to read from the websocket stop and return. In these cases, there were debug and error level logs which were not necessary because this was the normal mode of operation.
The first commit in this PR simply makes the client/controller proxy return a nil error when they fail to read from their respective websocket connections.
The second commit in this PR was done after taking a look at whether we could simplify the complicated logic when handling
ctx.Done()involving a mutex. It was thankfully easy to clean this up because we already handle things gracefully when the client connection is closed so we can simply close the client connection to clean things up.Finally, I noticed an edge case that wasn't handled and added a test for it. If the connection to the controller was closed unexpectedly, the model proxy would not handle this gracefully. Now if the controller proxy routine ends early, we handle this by stopping the model proxy.
Engineering checklist
Check only items that apply
Test instructions
Ran the relevant tests with
-raceand/or-count=1000. E.g.Notes for code reviewers
I suggest reviewing this PR by each commit individually to see the changes more clearly.