Flatten RPCError causes if they're also RPCErrors with the same code
#2083
Conversation
There was a problem hiding this comment.
I also migrated this to swift-testing since I was adding new tests
Sources/GRPCCore/RPCError.swift
Outdated
| var finalMessage = message | ||
| var finalMetadata = metadata | ||
| var nextCause = cause | ||
| while let nextRPCErrorCause = nextCause as? RPCError { |
There was a problem hiding this comment.
I don't think we should do this in a loop. Consider:
let error1 = RPCError(code: .unavailable, message: "error1")
let error2 = RPCError(code: .unavailable, message: "error2", cause: error1, flatteningCauses: false)
let error3 = RPCError(code: .unavailable, message: "error3", cause: error2, flatteningCauses: true)Creating error3 would flatten the whole tree, even though error2 didn't want to be flattened.
I also don't know whether it makes sense to expose this as an option. Is there any reason we shouldn't unconditionally do this if the cause is an RPCError and the code matches?
Also, I think we should overload the init with a cause: RPCError so we don't have to pay for the cast if we're already holding the RPCError.
There was a problem hiding this comment.
I can't really think of a reason why we wouldn't want to do this unconditionally other than flattening may potentially be expensive since we're creating new errors and dealing with strings.
If we do it unconditionally, then the loop would be unnecessary because all underlying causes would have been flattened already anyways.
And yeah good idea about the overload.
There was a problem hiding this comment.
I can't really think of a reason why we wouldn't want to do this unconditionally other than flattening may potentially be expensive since we're creating new errors and dealing with strings.
We're already creating a new RPCError so that part isn't an issue. Strings a potentially expensive but that's the tradeoff for more info I guess.
If we do it unconditionally, then the loop would be unnecessary because all underlying causes would have been flattened already anyways.
Correct.
We can always add an opt-out later if we think it's necessary.
## Motivation We want to be able to flatten `RPCError`s, and to do so we need to be able to merge the `Metadata` contained in each. ## Modifications This PR adds a helper function to merge one `Metadata` instance into another. ## Result Unblocks #2083 and also provides a potentially useful API for users. **- Note:** Because of the way `Metadata` has been implemented, we can have multiple _identical_ key-value pairs. This isn't ideal, as it's particularly feasible that we'll end up with multiple repeated identical pairs when merging two `Metadata`s. I think we should reconsider the backing data structure (using a set for example) or add a check before inserting to avoid this.
gjcairo
force-pushed
the
rpcerror-flatten
branch
from
8c7964f
to
da760ba
Compare
gjcairo
force-pushed
the
rpcerror-flatten
branch
from
da760ba
to
eacd3fa
Compare
gjcairo
changed the title
RPCError causesRPCError causes if they're also RPCErrors with the same code
Motivation
For errors happening deep in the task tree, we'd wrap them in many layers of
RPCErrors. This isn't particularly nice.Modifications
This PR changes the behaviour of the
RPCErrorinitialiser to flatten the cause as long as it's anRPCErrorwith the same status code as the wrapping error.Result
Friendlier errors.