Add structured handshake rejections - #401
Conversation
| | 'REJECTED_UNSUPPORTED_CLIENT'; | ||
| // Application-defined rejection details. Older peers ignore this | ||
| // optional field. | ||
| details?: { |
There was a problem hiding this comment.
What do you think about making this just extras: unknown? I think at this point we're at the application boundary and we dont need to be prescriptive about shape. This is up to the app dev.
There was a problem hiding this comment.
I think that also works but might lead to a lot of application specific structures and duplication that we would have to keep track of. I think having some structure enforced in a details package could lead to bit more conformity in the codebase later on especially when this is only being used for error cases. But let me know this is a easy change and more about style preferences of the team
There was a problem hiding this comment.
application specific structures and duplication that we would have to keep track of
Can you expand on this?
| // Application-defined rejection details. Older peers ignore this | ||
| // optional field. | ||
| details?: { | ||
| code: string; |
There was a problem hiding this comment.
As I understand it's a bit of an anti pattern in river to have code: string. It tends to prefer specified union types (see code in the layer above)
| | 'REJECTED_UNSUPPORTED_CLIENT'; | ||
| // Application-defined rejection details. Older peers ignore this | ||
| // optional field. | ||
| details?: { |
There was a problem hiding this comment.
Does this details structure fit with every code or just with REJECTED_BY_CUSTOM_HANDLER? If not the type is currently a bit leaky (in that it specifies that details belongs with all codes - and maybe thats intented)
2 participants
Why
Custom handshake handlers can only return a River failure code. Applications must put specific failure states in error strings and parse those strings for retry decisions. We can log application specific errors which will improve logging and error handling flows.
Current specific use case is for terminal errors happening on river handshakes we want to close the river connection we have no way to differentiate between a handshake error that is terminal vs error retry
What changed
Custom handshake handlers can now return
rejectHandshake({ code, message, extras }). River sends these optional details in failed handshake responses and exposes them in protocol error events and logs. Existing handlers that return a River failure code continue to work without changes.Tests cover initial handshakes, re-handshakes, and compatibility with old clients and servers. The protocol and handshake documentation describe the new payload.
Versioning
~ written by Zerg 馃懢 (wp-8f1d1ac5)