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

No error details when exceeding of LengthDelimitedCodec's max frame length #389

Closed
fspreiss opened this issue Jan 9, 2023 · 6 comments · Fixed by #399
Closed

No error details when exceeding of LengthDelimitedCodec's max frame length #389

fspreiss opened this issue Jan 9, 2023 · 6 comments · Fixed by #399
Assignees
Labels

Comments

@fspreiss
Copy link

fspreiss commented Jan 9, 2023

We have an RPC service that takes a parameter of type Vec<u8>. Also, we use a LengthDelimitedCodec (as framed_io) in a serde transport (created with tarpc::serde_transport::new) in our client (created with tarpc::client::new).

If this service is called with the Vec<u8> parameter being larger than the LengthDelimitedCodec's max frame length (by default, this is 8MB), then this leads to the client disconnecting and returning an RpcError::Disconnected ("the client disconnected from the server").

The error (message) is somewhat suboptimal because it doesn't give any indication on what the actual problem is (i.e., that the problem comes from exceeding the max frame length). In our case, it took several days of debugging (a production incident) to figure out what the problem is.

Would it maybe possible to improve the situation, e.g., by having a more detailed/specific error that directly points to the actual underlying problem?

@tikue tikue self-assigned this Jan 10, 2023
@tikue tikue added the bug label Jan 10, 2023
@tikue
Copy link
Collaborator

tikue commented Jan 10, 2023

Thank you for raising this! I'm sorry it made incident mitigation more difficult.

Just to double check, the limit you ran into was the client-side limit? I think I can pretty easily add a better error message when the client stops an RPC from being sent. Here's a prototype:

2023-01-10T02:49:32.528939Z  WARN client: the client failed to send the request

Caused by:
    0: could not write to the transport
    1: frame size too big

(On the server side, it's more complicated for the server to remediate the byte stream if there is a lot of junk on the stream from a request that is too large.)

@fspreiss
Copy link
Author

Yes, the limit we ran into was on the client-side, because a parameter of a request was too big.

I just confirmed this with a test that sets the max frame size to something very low on the client and something very high on the server, and my test failed with the mentioned RpcError::Disconnected on the client side. (Typically we have the same limits on both the client and the server, however.)

That being said, I could imagine a similar issue happening on the server if a response gets too big.

Thanks for creating a prototype so quickly! An error message containing such specific error details ("frame size too big") in a new enum variant (RpcError::Send in your prototype) would definitely be very helpful (and would certainly have helped in our debugging).

@bruwozniak
Copy link
Contributor

@tikue any chance of moving forward with the prototype? I am looking at a similar issue where it's hard to understand the underlying cause. It would be very helpful to have this kind of information available

@tikue
Copy link
Collaborator

tikue commented Mar 10, 2023

Hey, sorry for the delay here! I've been busy and haven't had time to work on this. I'd be happy for someone else to pick it up. The main thing missing from the above was tests.

@bruwozniak
Copy link
Contributor

@tikue thanks for replying, I have cherry picked your commits and added a test. Let me know if the direction looks good and if anything else is missing.

@tikue
Copy link
Collaborator

tikue commented Mar 24, 2023

I have published the fix in v0.32.0. Thanks again!

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

Successfully merging a pull request may close this issue.

3 participants