Make remote call errors configurable #1020
Conversation
| case .all: // compiler gets confused if this is grouped together with above | ||
| reply = .init(callID: callID, error: codableError) | ||
| default: | ||
| reply = .init(callID: callID, error: GenericRemoteCallError(message: "Remote call error of [\(type(of: error as Any))] type occurred")) |
There was a problem hiding this comment.
does this emit a warning? May have to be any Any? (not sure, just asking)
There was a problem hiding this comment.
I haven't noticed any. Will double check.
There was a problem hiding this comment.
Ok cool! Learning the new rules about spelling types myself still huh
| import DistributedActorsTestKit | ||
| import XCTest | ||
|
|
||
| final class RemoteCallTests: ClusteredActorSystemsXCTestCase { |
| guard error is GenericRemoteCallError else { | ||
| throw TestError("Expected GenericRemoteCallError, got \(error)") | ||
| } | ||
| } |
There was a problem hiding this comment.
can we add another test when we allow-listed an error, but did not register it?
There was a problem hiding this comment.
In that case we'd get a timeout error IIRC. What's the expected behavior? I guess it should return a serialization error -> generic error.
There was a problem hiding this comment.
We get timeout error because we fail to deserialize AnyRemoteCallReply (TransportPipelines.swift L641) so we can't even call back.
| let remote = await setUpNode("remote") { settings in | ||
| settings.remoteCall.codableErrorAllowance = .custom([AnotherGreeterCodableError.self]) | ||
| settings.serialization.registerInbound(GreeterCodableError.self) | ||
| settings.serialization.registerInbound(AnotherGreeterCodableError.self) |
There was a problem hiding this comment.
Hm I'm wondering if this is too annoying... we have to allow list types, that's for sure... and serializers as well... Hm, for now this is okey, maybe some sugar in the future "register trusted error type"?
There was a problem hiding this comment.
Maybe I still don't get the directions right, but for me it's confusing in that I don't know on which node I need to set the allow list and register types (or maybe there should be registerInbound and registerOutbound?). I feel like I should only need to do these on the responding node.
Hm, for now this is okey, maybe some sugar in the future "register trusted error type"?
We can probably make ClusterSystemSettings smarter and have it automatically register the allowed types?
There was a problem hiding this comment.
Hmm, how come GenericRemoteCallError doesn't need to be registered?
There was a problem hiding this comment.
Yeah I agree this is confusing, I'll have a look
ktoso
left a comment
ktoso
left a comment
There was a problem hiding this comment.
LGTM, one potential follow up and extra test but looks good 👍
|
Would you be up to writing a small docs section about those as well actually? It'd be in |
Sure. I will update the docs in this PR as well. |
2 participants
Resolves #932