Improve errors and edge case handling in model proxy #1336
Closed
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
- 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
fb3842d to
4b5c1a7
Compare
kian99
requested review from
alesstimec,
SimoneDutto,
babakks,
ale8k and
pkulik0
SimoneDutto
reviewed
Sep 3, 2024
| @@ -339,8 +332,8 @@ func (p *clientProxy) start(ctx context.Context) error { | |||
| zapctx.Debug(ctx, "Reading on client connection") | |||
| msg := new(message) | |||
| if err := p.src.readJson(&msg); err != nil { | |||
There was a problem hiding this comment.
reading at gorilla/websocket/json.go can return an error for two reasons:
- error on marshaling the json
- error on closing the connection.
I think your PR wants to hide error on closing connection, and that is fine from what I understand, but we are also masking errors on json marshaling, which could potentially lead to situation where the clients sends us non-json messages and we are not seeing the error.
Wouldn't be better to check here for marshaling error? And if not ignoring?
There was a problem hiding this comment.
Fixed in #1345, I tried updating this PR but Github just hangs, I believe because this PR is still from my old private fork of JIMM.
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 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.