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

Bubble up server side transport errors to the client #403

Open
axos88 opened this issue Apr 10, 2023 · 5 comments
Open

Bubble up server side transport errors to the client #403

axos88 opened this issue Apr 10, 2023 · 5 comments

Comments

@axos88
Copy link

axos88 commented Apr 10, 2023

I think #399 is a step in the right direction, however currently if the server fails to handle a request from the client, the client is never notified, the request simply times out.

For example an out-of-date client can send the wrong message to the server, causing a serde deserialization error. This is logged on the server side, but the client is never notified. I'd expect the client to get the error message as well.

@axos88
Copy link
Author

axos88 commented Apr 10, 2023

@bruwozniak @tikue

@tikue
Copy link
Collaborator

tikue commented Apr 10, 2023

Yes, agreed! Though this would be a larger change, and I think there some open questions about what would even be a reasonable thing for the server to do in this case.

For example, to preserve the integrity of the TCP stream, the server would need to read all the remaining bytes for the request, to /dev/null or something. This would be a change that needs to be supported at the transport level rather than the application level, and the serde transport commonly used doesn't support this capability.

@axos88
Copy link
Author

axos88 commented Apr 10, 2023

It's true that it's definately not something all transports can support. I'm using tarpc over MQTT which is message-based, so continuing the stream with the next message is not a problem, but in case of a TCP stream it may or may not be able to be easily implemented without risking the integrity of the connection.

But we need to differentiate between incorrect and invalid requests. The former - in theory - shouldn't be a problem even for a stream-based transport. serde-json can for example know where the object starts and ends { ... }, even if it is unable to correctly deserialize it. (missing/unknown field, wrong value type). It would be however hard to continue a stream where the received blob is not a valid json string. This distinction cannot be made for all transports though - just by changing the codec to say bincode the two cannot be realistically distingouished from another.
The distinction however is trivial for message based transports though.

@axos88
Copy link
Author

axos88 commented Apr 10, 2023

But even in case of the serde-binpack, we could basically wrap it in a codec that would prefix the data with the length, and suffix it with a checksum, thus we can validate the frames from eachother.

And I think we're arriving at a point where we'd be introducing another layer of abstraction - encoding of the data on the transport.

Transport being http / mqtt / websocket / whatever, and encoding being the frame format (json, bincode, etc).

I'm not saying this is something tarpc would need to do, but I just realized while writing this message that currently we are conflating these two abstractions.

@tikue
Copy link
Collaborator

tikue commented Feb 4, 2024

You're right, and you might even separate framing into its own layer so that there are three things conflated in the transport: message serialization (bincode, json, etc), message framing (header length, big endian or little endian, etc), and byte streaming.

Of course, that's all assuming networking is even happening! One of the cool things about tarpc is that serialization etc is all opt-in. Tarpc can be used to connect different in-process services where everything is done with rust channels.

One question: I assume this would still be treated as a permanent error on the client side? For example, if decoding the request fails, the server will not know what the request ID was, and the client will basically have no choice but to spray the error to all in flight requests?

I was recently thinking about a related feature for the server to tell a client to go away, for example, if the server is shutting down. If the go-away message contained a payload, it could also be utilized for cases like this, where the server isn't shutting down but wants a bad-acting client to go away.

@tikue tikue closed this as completed Feb 4, 2024
@tikue tikue reopened this Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants