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

Improves application error handling by introducing error code/id in payload #282

Open
OlegDokuka opened this issue Dec 5, 2019 · 12 comments
Assignees

Comments

@OlegDokuka
Copy link
Member

Problematic

Right now, we have a set of clearly defined Error Types, which make it easy to handle different exceptions on the level of RSocket-Protocol. On of the available RSocket error types is the APPLICATION_ERROR type. Apart from that, every error has its own error data, which seems to be a good place to define what exactly happened. So all this information is defined by the specification like the following:

Frame Contents

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                           Stream ID                           |
    +-----------+-+-+---------------+-------------------------------+
    |Frame Type |0|0|      Flags    |
    +-----------+-+-+---------------+-------------------------------+
    |                          Error Code                           |
    +---------------------------------------------------------------+
                               Error Data

Unfortunately, in the current error definition, there is a couple of significant gaps:

  1. Firstly, the error codes mostly are for internal RSocket usage.

  2. Application Error is generic enough to be useless for application needs especially when the content is UTF-8 based message

  3. There is a set of error codes allocated for application needs, but there is no definition for a set of WellKnowApplicationErrors which simplify general communication

Proposal

I propose to provide an extension for WellKnownApplicationErrors so the application developer can use them in order to define/recognize what happened in a stream easily

@OlegDokuka OlegDokuka self-assigned this Dec 5, 2019
@yschimke
Copy link
Member

yschimke commented Dec 6, 2019

What is the representative initial set you would suggest? Would rsocket-java and rsocket-cpp both implement these initial ones as provided classes?

@OlegDokuka
Copy link
Member Author

@yschimke, not an expert in the area of error codes, but I'm thinking about a similar document to that one -> https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html.

I guess there is no sense to port HTTP specific error codes, but I guess:

WellKnown Error Codes

Type Value Description
APPLICATION_ERROR 0x00000201 Application layer logic generating a Reactive Streams onError event. Stream ID MUST be > 0.
ROUTE_NOT_FOUND 0x00000301 Notify that the reponder does not support given route. Stream ID MUST be > 0.
UNSUPPORTED_REQUEST_DATA_MIME-TYPE 0x00000302 Notify that the responder does not support given request data mime-type. Stream ID MUST be > 0.
UNSUPPORTED_RESPONSE_DATA_MIME-TYPE 0x00000303 Notify that the responder does not support given response data mime-type. Stream ID MUST be > 0.
AUTHENTICATION_REQUIRED 0x00000401 Notify that the requester must provide an authentication in order to continue interaction. Stream ID MUST be > 0.
INVALID_CREDENTIALS 0x00000402 Notify that the given authentication is invalid. Stream ID MUST be > 0.
UNAUTHORIZED_ACCESS 0x00000403 Notify that the requester is authenticated however is not authorized to make the request. Stream ID MUST be > 0.

that what came to mime mind so far. Any suggestions are welcome

cc/ @rstoyanchev @yschimke @linux-china

@SerCeMan
Copy link
Member

SerCeMan commented Apr 16, 2020

Would it be reasonable to change "Error Data" to be an arbitrary payload, not UTF-8 encoded?

I see a potential need for having human-readable messages though as it perfectly maps to exceptions. So, alternatively, maybe it's possible to transform "Error Data" to "Error Message" which can continue to be UTF-8 encoded while allowing for arbitrary payloads.

The reason is that there is not always a way to encode an error payload as a UTF-8 string or, to be precise, it's simply not the most optimal encoding.

@JamesChenX
Copy link

JamesChenX commented Nov 18, 2020

Hi @OlegDokuka, is there any progress on this issue? The issue has been suspended for a year.
The current serialization implementation of ErrorFrameCodec isn't reasonable because it serializes a Throwable instance by getMessage() while getMessage() is always used for logging, which means getMessage() is used for both serialization and logging.

And there should be a custom serializer exposed for developers to serialize ApplicationErrorException. getMessage() should only be used if developers don't specify a custom serializer.

The behavior of error payload is very important for any robust system.

@jiraguha
Copy link

At first thought, forced "error data" to be a UTF-8 encoded string only seems to be arbitrary and limiting! see error-frame-0x0b.

I follow @SerCeMan about the question of having "Error Data" as an arbitrary payload!... What is missing to move on this problem?

If we use an arbitrary payload, we could have more freedom about how to manage exception passing from one application to another via the protocol.

In java, errors of type APPLICATION_ERROR are mapped to ApplicationErrorException which contains a message and an empty cause (that was left out due to this protocol limitation).

An idea to a bit workaround the limit could be to use that frame for sending back unique business code (easily parsable) that match the root cause and we could advise the client from there.

But sometimes you want to send more specific information to contextualize the error like the id of the resource causing the trouble and in my knowledge of rsocket, I see nothing to send back that contextual info to the client, except using "Error Data", with again, its narrow the possibilities.

My use-case stays ideal as I speak about managing 1 error only. In a general case, we have several errors that can be originated from the responder. In the graphQL, for example, they represent an error by a set of complex objects.

@OlegDokuka OlegDokuka added this to the 2.0 milestone Nov 29, 2020
@jiraguha
Copy link

jiraguha commented Dec 2, 2020

Meanwhile, @UpyEngineering I am working on "URL based spec/convention" for managing errors for those how are interested.

After analysis, I notice that it could feet most use cases.

It is still at the draft level so your inputs are welcome.

@OlegDokuka
Copy link
Member Author

OlegDokuka commented Dec 2, 2020

@jiraguha I would recommend to "keep it simple". In rsocket, we do not have any concept of the URL and we are not planning to add one.

The minimal change that we want - is to make data binary.
Then, having the concept of data/metadata mime-type on the connection level / or using extension for per-request mime-type one can agree on the content format and use it for both - Payload and ErrorPayload content encoding/decoding.

Also, please keep in mind that most likely you are doing something wrong if you want to send your logical data response in your Exception.
Of course, we do that but usually, it is a limited use case, and what we do instead is sending a normal response which can be treated on the application level as an error payload. The prevailing case on how we use our Exceptions in reality - is reading the String encoded message and logging that message somewhere. That is why initially data in the ErrorPayload stod for UTF-8 based content and nothing else.

Apart, keep in mind that spec is cool, but another challenge - is implementation, which may not be able to implement the proposed idea easily (or in the worth case - at all)

@OlegDokuka
Copy link
Member Author

OlegDokuka commented Dec 2, 2020

Also, apart from the error data, the goal was to extend the number of error codes to be a set of well-known error codes for RSocket.

In the distributed environment and edge network, well-known error codes can simplify life be strictly saying that error code (e.g. 0x0000401 stands for Authentication Error)

@jiraguha
Copy link

jiraguha commented Dec 2, 2020

@OlegDokuka Thanks for your feedback your recommendations are highly valuable to me,

I do not understand your point about logical data response. Can you elaborate? Is WellKnown error codes also a way to send information (or logical data) in the error frame. In the end, depending on your use-case you will want to send more or less info to the client... The client can do more or fewer actions like error advising or just logs.

Just for the example, Validations is one use-case that happened not so rare in an application lifecycle where a client could need the exact names of the fields that cause the error to react consequently or insure an error translation for an e2e customers. zalando problems are good examples of how we would expect an application server to behave.

Pushing error data to Exception or moving them at application-level is a matter of implementation choice that application designers should take free-rely depending on the project requirement. Personally, I am not a fan of treating an error as a normal application payload because to me that is going out of the error semantic. Moreover, it demands extra effort to create middle-ware that would parse the payload and detect errors.

"UTF8 only" limitation and simplicity are the reason why we would use URL as an abstraction for errorData. It could be easy to implement and to test as many libs exist for that.

Keep in mind that we do not want to write something that could go against Rsocket vision and design. Make data binary would be awesome and will open possibilities. meanwhile, we investigating time and effort to have an "easy to implement workaround" that could go into production and respond to our business requirements.

One more thing @OlegDokuka, does rsocket have a Slack channel where we can push further those kinds of reflection?

@kitkars
Copy link

kitkars commented Mar 4, 2021

Hi @OlegDokuka ,
Is it already implemented? Do you any samples?

Thanks.

@OlegDokuka
Copy link
Member Author

OlegDokuka commented Mar 4, 2021

@kitkars it is not. And it will not probably be a part of the spec, since in reactive stream onError signal has a different meaning than the application level error.

onError signal created by Reactive Stream should be treated as something unexpected terminating an execution.

If you would like to send a message, which then can be treated as an error by the application logic, it is better to send it as a normal onNext signal.

@OlegDokuka OlegDokuka removed this from the 2.0 milestone Mar 4, 2021
@kitkars
Copy link

kitkars commented Mar 4, 2021

Thanks @OlegDokuka .

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

No branches or pull requests

6 participants