=serialization #512 Finish ErrorEnvelope implementation #549

yim-lee · 2020-04-08T19:46:56Z

Motivation:
ErrorEnvelope's handling of Codable errors not yet implemented.

Modifications:
Finish implementing ErrorEnvelope. Frequently ran into the error below and had to take a different approach.

value of protocol type 'Codable' (aka 'Decodable & Encodable') cannot conform to 'Encodable'; only struct/enum/class types can conform to protocols

Results:
Resolves #512

Motivation: `ErrorEnvelope`'s handling of `Codable` errors not yet implemented. Modifications: Finish implementing `ErrorEnvelope`. Frequently ran into the error below and had to take a different approach. ``` value of protocol type 'Codable' (aka 'Decodable & Encodable') cannot conform to 'Encodable'; only struct/enum/class types can conform to protocols ``` Results: Resolves #512

yim-lee · 2020-04-08T19:47:56Z

Sources/DistributedActors/Serialization/Serialization+SerializerID.swift

        internal static let AckOps: SerializerID = .foundationJSON
+
+        internal static let ErrorEnvelope: SerializerID = .foundationJSON
+        internal static let BestEffortStringError: SerializerID = .foundationJSON


It doesn't seem like this change is required but adding them here to be consistent

Right yeah because we default to JSON so it works fine by using it.

Thanks for adding here 👍

yim-lee · 2020-04-08T19:49:33Z

Sources/DistributedActors/Serialization/Serialization+Settings.swift

    /// By doing this before system startup you can ensure a specific serializer is used for those messages.
    /// Make sure tha other nodes in the system are configured the same way though.
-    public mutating func registerCodable<Message: ActorMessage>(
+    public mutating func registerCodable<Message: Codable>(


Unrelated to the PR.

Even though ActorMessage is a type alias of Codable, using Codable explicitly here to better match method name (similar to registerProtobufRepresentable)

Right ok, thankfully I'm able to remove both those specialized functions soon :)

yim-lee · 2020-04-08T19:50:46Z

Sources/DistributedActors/ActorMessages.swift

+    public typealias CodableError = Error & Codable

-    public init<Failure: Error>(_ error: Failure) {
+    private let _boxed: BoxedCodableError


I think this is cleaner and so we can avoid doing type checks everywhere, plus BestEffortStringError is Codable.

yim-lee · 2020-04-08T19:52:30Z

Sources/DistributedActors/ActorMessages.swift

-        // FIXME: implement being able to encode and carry Codable errors (!)
-//        if let codableError = self.error as? Codable {
-//            try container.encode(context.outboundManifest(type(of: self.error as Any)), forKey: .manifest)
-//            try container.encode(codableError, forKey: .error)


I couldn't get this approach to work because Codable is a protocol:

value of protocol type 'Codable' (aka 'Decodable & Encodable') cannot conform to 'Encodable'; only struct/enum/class types can conform to protocols

Right, what you dod though is absolutely right 👍

yim-lee · 2020-04-08T19:53:57Z

Sources/DistributedActors/ActorMessages.swift

+                throw SerializationError.unableToDeserialize(hint: "Error type \(errorType) is not Codable")
+            }
+
+            let errorDecoder = try container.superDecoder(forKey: .error)


Found this by serendipity. Seems to work (according to the tests).

yim-lee · 2020-04-08T19:59:47Z

Test failure: #550

@swift-server-bot test this please

yim-lee · 2020-04-08T23:27:56Z

Sources/DistributedActors/ActorMessages.swift

+    }

-        let container = try decoder.container(keyedBy: CodingKeys.self)
+    private struct BoxedCodableError: Codable {


Actually don't need separate type. Removed.

ktoso · 2020-04-09T02:31:07Z

Sources/DistributedActors/ActorMessages.swift

-// FIXME: Needs better impl: https://github.com/apple/swift-distributed-actors/issues/512
 public struct ErrorEnvelope: Error, ActorMessage {
-    public let error: Error
+    public typealias CodableError = Error & Codable


ktoso · 2020-04-09T02:53:35Z

Sources/DistributedActors/ActorMessages.swift

+        }
+
+        let errorDecoder = try container.superDecoder(forKey: .error)
+        self.codableError = try codableErrorType.init(from: errorDecoder)


oh interesting

ktoso

Awesome, thanks a lot :)

ktoso · 2020-04-09T02:53:58Z

Sources/DistributedActors/ActorMessages.swift

-        // FIXME: implement being able to encode and carry Codable errors (!)
-//        if let codableError = self.error as? Codable {
-//            try container.encode(context.outboundManifest(type(of: self.error as Any)), forKey: .manifest)
-//            try container.encode(codableError, forKey: .error)


Right, what you dod though is absolutely right 👍

ktoso · 2020-04-09T02:54:39Z

Sources/DistributedActors/Serialization/Serialization+SerializerID.swift

        internal static let AckOps: SerializerID = .foundationJSON
+
+        internal static let ErrorEnvelope: SerializerID = .foundationJSON
+        internal static let BestEffortStringError: SerializerID = .foundationJSON


Right yeah because we default to JSON so it works fine by using it.

Thanks for adding here 👍

ktoso · 2020-04-09T02:55:56Z

Sources/DistributedActors/Serialization/Serialization+Settings.swift

    /// By doing this before system startup you can ensure a specific serializer is used for those messages.
    /// Make sure tha other nodes in the system are configured the same way though.
-    public mutating func registerCodable<Message: ActorMessage>(
+    public mutating func registerCodable<Message: Codable>(


Right ok, thankfully I'm able to remove both those specialized functions soon :)

ktoso · 2020-04-09T02:58:41Z

Sources/DistributedActors/Serialization/Serialization.swift


+        // errors
+        settings.registerCodable(ErrorEnvelope.self) // TODO: can be removed once https://github.com/apple/swift/pull/30318 lands
+        settings.registerCodable(BestEffortStringError.self) // TODO: can be removed once https://github.com/apple/swift/pull/30318 lands


yim-lee requested a review from ktoso April 8, 2020 19:46

yim-lee commented Apr 8, 2020

View reviewed changes

yim-lee mentioned this pull request Apr 8, 2020

FAILED: CRDTActorOwnedTests.test_actorOwned_ORSet_add_many_times #550

Closed

Remove BoxedCodableError type

0a066cf

yim-lee commented Apr 8, 2020

View reviewed changes

ktoso reviewed Apr 9, 2020

View reviewed changes

ktoso approved these changes Apr 9, 2020

View reviewed changes

ktoso merged commit 79ebc89 into apple:master Apr 9, 2020

ktoso deleted the error-envelope-1 branch April 9, 2020 02:59

=serialization #512 Finish ErrorEnvelope implementation #549

=serialization #512 Finish ErrorEnvelope implementation #549

Uh oh!

Conversation

yim-lee commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yim-lee commented Apr 8, 2020 •

edited

Loading