search

eclipse-uprotocol / up-rust

uProtocol Language Specific Library for Rust
Apache License 2.0
11 stars 9 forks source link

UListener::on_error should provide message context #87

Closed sophokles73 closed 2 months ago

sophokles73 commented 5 months ago

Currently UListener::on_error(UStatus) only provides error details in the UStatus struct but does not provide any information regarding the context in which the error occurred.

It seems reasonable to either pass in a full UMessage with the UStatusas payload or instead pass in both UStatus and UAttributes.

sophokles73 commented 5 months ago

Personally, I would be in favor of passing in a UMessage, which would allow a UTransport implementation to do both, either pass in a UMessage returned from a service provider as is or manually create a UMessage in case of a transport error.

WDYT @PLeVasseur @AnotherDaniel ?

evshary commented 5 months ago

I'm not sure, but at least using UStatus is enough for me now. I can add some messages in the UStatus to describe what kind of error is. For example, this is how it works in up-client-zenoh-rust: https://github.com/ZettaScaleLabs/up-client-zenoh-rust/blob/680dc87ea334ae54e07a361a8d2138f7aac70d89/src/utransport.rs#L268

Indeed passing UMessage allows users to do more things in on_error, but it's a little cumbersome if we don't have UMessage and need to create an empty one to return. (Maybe using Option instead?)

PLeVasseur commented 5 months ago

Indeed passing UMessage allows users to do more things in on_error, but it's a little cumbersome if we don't have UMessage and need to create an empty one to return. (Maybe using Option instead?)

I think this is a good point. Could be that for some internal error reason there is no UMessage available.

Feels a little weird for the API to be UListener::on_error(Result<UMessage, UStatus>), but I suppose it could allow for handling cases where the implementor of UTransport is unable to provide the UMessage.

evshary commented 5 months ago

Feels a little weird for the API to be UListener::on_error(Result<UMessage, UStatus>), but I suppose it could allow for handling cases where the implementor of UTransport is unable to provide the UMessage.

How about this? UListener::on_error(UStatus, Option<UMessage>) Users only need to provide UMessage if there is a valid UMessage

sophokles73 commented 5 months ago

The idea would be to have

fn on_error(error: UMessage);

The message would either come directly from the service implementation or the transport would create it manually if something goes wrong, e.g. when forwarding a request message. In the latter case, the transport could copy the request message's UAttributes (to provide for context), swap the source and dest and set a commstatus other than OK. It could then add a UStatus as the message payload.

WDYT?

evshary commented 5 months ago

IMHO, I think extracting UStatus from UPayload is a little not intuitive and it would mix with the original UPayload. It would be better to separate them.

sophokles73 commented 5 months ago

IMHO, I think extracting UStatus from UPayload is a little not intuitive and it would mix with the original UPayload. It would be better to separate them.

That is exactly what an application would need to do in case the response sent from the service contains an error, wouldn't it?

evshary commented 5 months ago

Do you mean we don't keep the origin UPayload and swap it with UStatus? I'm still not clear about how to put the original UPayload and UStatus together.

sophokles73 commented 2 months ago

As part of implementing the L2 RpcClient we have removed the UListener::on_error function which does not serve any purpose (anymore). See d49ee8e8e202a0f23579d33bfae36da699e7fd4b