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: check decode error #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hunjixin
Copy link
Contributor

No description provided.

Comment on lines +620 to +621
c.incomingErr = err //must set incomingErr, tell the request fail immediately
close(c.incoming)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to drop the connection, we should just call c.conn.Close and return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if ignore this error. a request may send to to unstable connection, but never get a response

  1. got decode error and tryReconnect. (inflight was removed) but not success
  2. a request send before tryReconnect success(conn not change to new one), send can success because the network was not comlete unreachable
  3. tryReconnect success, and change a new conn, but the request in step 2 will never get response
  4. client request get stuck for ever

@Fatman13
Copy link

bump

@rjan90
Copy link

rjan90 commented Jan 7, 2025

Hey! 👋

As part of our cleanup to kick off the year, I'm reviewing all open non-draft pull requests. Could you please do one of the following for your PR?

1. Close it: If it's no longer needed.
2. Mark as Draft: If it needs more work, and add the next steps.
3. Ready for Review: If it's good to go, let me know, and I'll assign a reviewer.

If there's no response in a week, I'll assume it's option 1 and close the PR. If you have any questions, just let me know.

Thanks for your help in keeping things organized, and I appreciate your contributions!

@rvagg
Copy link
Member

rvagg commented Jan 7, 2025

This is quite out of date now, the current code handles the error with a log and moves on:

go-jsonrpc/websocket.go

Lines 691 to 696 in 9a16432

if err := json.Unmarshal(buf, &frame); err != nil {
log.Warnw("failed to unmarshal frame", "error", err)
// todo send invalid request response
continue
}

Dropping the connection and returning an error is a reasonable thing to also do, but not necessary I think and also a bit difficult given that this is done in a separate goroutine now. I think this can just be closed and if @hunjixin wants to open a new PR that changes the current behaviour we can consider that separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants