Skip to content

=serialization rm 5.2.4 workaround, fix LWWMap with Codable Value (Codable in Proto nesting) #782

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 15, 2020
Merged

=serialization rm 5.2.4 workaround, fix LWWMap with Codable Value (Codable in Proto nesting) #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2e8d4b
=serialization remove workaround
ktoso Oct 14, 2020
e86b29e
=fix,ser serialization would crash if LWWMap used a value that was Co…
ktoso Oct 14, 2020
f494b78
debug CI; this works locally in docker swift 5.2.5 + linux...
ktoso Oct 14, 2020
85931a7
dont install jazzy on xenial
ktoso Oct 15, 2020
8a0feff
version
ktoso Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 0 additions & 39 deletions Sources/DistributedActors/Serialization/Serialization+Codable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,42 +80,3 @@ extension Encodable {
try encoder.encode(self)
}
}

// TODO: could not get it to work
//// ==== ----------------------------------------------------------------------------------------------------------------
//// MARK: encode with manifest
//
// extension KeyedEncodingContainerProtocol {
// mutating func encode<T>(_ value: T, forKey key: Self.Key, forManifestKey manifestKey: Self.Key) throws where T: Encodable {
// let encoder = self.superEncoder()
// guard let context: Serialization.Context = encoder.actorSerializationContext else {
// throw SerializationError.missingSerializationContext(encoder, value)
// }
//
// let serialized = try context.serialization.serialize(value)
//
// try self.encode(serialized.manifest, forKey: manifestKey)
// try self.encode(serialized.buffer.readData(), forKey: key)
// }
//
//
// }
//
// extension KeyedDecodingContainerProtocol {
// func decode<T>(
// _ type: T.Type, forKey key: Self.Key, forManifestKey manifestKey: Self.Key,
// file: String = #file, line: UInt = #line
// ) throws -> T where T: Decodable {
// let decoder = self.superEncoder()
//
// guard let context: Serialization.Context = decoder.actorSerializationContext else {
// throw SerializationError.missingSerializationContext(T.self, details: "Missing context", file: file, line: line)
// }
//
// let manifest = try self.decode(Serialization.Manifest.self, forKey: manifestKey)
// let data = try self.decode(Foundation.Data.self, forKey: manifestKey)
//
// return try context.serialization.deserialize(as: T.self, from: .data(data), using: manifest)
// }
//
// }
Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, cleanup

8 changes: 4 additions & 4 deletions Sources/DistributedActors/Serialization/TopLevelProtobufCoders.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import NIOFoundationCompat
// ==== ----------------------------------------------------------------------------------------------------------------
// MARK: Top-Level Bytes-Blob Encoder

// TODO: TopLevelDataEncoder
Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually kind of implements just that I guess :nod:


final class TopLevelProtobufBlobEncoder: _TopLevelBlobEncoder {
let allocator: ByteBufferAllocator

Expand Down Expand Up @@ -78,7 +76,7 @@ final class TopLevelProtobufBlobEncoder: _TopLevelBlobEncoder {

func unkeyedContainer() -> UnkeyedEncodingContainer {
fatalErrorBacktrace("Attempted \(#function) in \(self)")
// TopLevelProtobufBlobEncoderContainer(superEncoder: self)
// TopLevelProtobufBlobUnkeyedEncodingContainer(superEncoder: self)
Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though we don't actually need it, good

}

func singleValueContainer() -> SingleValueEncodingContainer {
Expand All @@ -100,7 +98,9 @@ struct TopLevelProtobufBlobSingleValueEncodingContainer: SingleValueEncodingCont
case let repr as AnyProtobufRepresentable:
try repr.encode(to: self.superEncoder)
case let data as Data:
try data.encode(to: self.superEncoder)
try self.superEncoder.store(data: data)
case let bytes as [UInt8]:
try self.superEncoder.store(bytes: bytes)
Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we're not really "encoding" we're just using it and that's great 👍

Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix™

default:
throw SerializationError.unableToSerialize(hint: "Attempted encode \(T.self) into a \(Self.self) which only supports ProtobufRepresentable")
}
Expand Down
83 changes: 61 additions & 22 deletions Tests/DistributedActorsTests/CRDT/CRDTGossipReplicationClusteredTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,25 @@ final class CRDTGossipReplicationClusteredTests: ClusteredActorSystemsXCTestCase

override func configureActorSystem(settings: inout ActorSystemSettings) {
settings.serialization.register(CRDT.ORSet<String>.self)
settings.serialization.register(CRDT.LWWMap<String, ValueHolder<String?>>.self)
settings.serialization.register(CRDT.LWWRegister<ValueHolder<String?>>.self)
settings.serialization.register(ValueHolder<String?>.self)
settings.serialization.register(CRDT.LWWMap<String, String?>.self)
settings.serialization.register(CRDT.LWWRegister<String?>.self)
settings.serialization.register(CodableExampleValue.self)
settings.serialization.register(CRDT.LWWMap<String, CodableExampleValue>.self)
settings.serialization.register(CRDT.LWWRegister<CodableExampleValue>.self)
Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit annoying - but good reminder, to use a LWWMap one has to use a LWWRegister heh :~

settings.serialization.register(String?.self)
}

struct CodableExampleValue: Equatable, Codable, ExpressibleByStringLiteral {
let value: String

init(stringLiteral value: StringLiteralType) {
self.value = value
}
}

// ==== ----------------------------------------------------------------------------------------------------------------
// MARK: Owns Set

enum OwnsSetMessage: NonTransportableActorMessage {
case insert(String, CRDT.OperationConsistency)
}
Expand All @@ -58,6 +72,9 @@ final class CRDTGossipReplicationClusteredTests: ClusteredActorSystemsXCTestCase
}
}

// ==== ----------------------------------------------------------------------------------------------------------------
// MARK: Owns Counter

func ownsCounter(p: ActorTestProbe<CRDT.GCounter>?) -> Behavior<Int> {
.setup { context in
let counter: CRDT.ActorOwned<CRDT.GCounter> = CRDT.GCounter.makeOwned(by: context, id: "counter")
Expand All @@ -72,6 +89,9 @@ final class CRDTGossipReplicationClusteredTests: ClusteredActorSystemsXCTestCase
}
}

// ==== ----------------------------------------------------------------------------------------------------------------
// MARK: Owns Map

enum OwnsMapMessage<Value: Codable>: NonTransportableActorMessage {
case set(key: String, value: Value, CRDT.OperationConsistency)
}
Expand Down Expand Up @@ -123,15 +143,36 @@ final class CRDTGossipReplicationClusteredTests: ClusteredActorSystemsXCTestCase
try self.expectSet(probe: p2, expected: ["a", "aa", "b"])
}

struct ValueHolder<Value: Codable & Equatable>: Codable, Equatable {
let value: Value?

init(_ value: Value? = nil) {
self.value = value
func test_gossip_localLWWMapUpdate_toOtherNode() throws {
let configure: (inout ActorSystemSettings) -> Void = { settings in
settings.crdt.gossipInterval = .seconds(1)
settings.crdt.gossipIntervalRandomFactor = 0 // no random factor, exactly 1second intervals
}
let first = self.setUpNode("first", configure)
let second = self.setUpNode("second", configure)
try self.joinNodes(node: first, with: second, ensureMembers: .up)

let p1 = self.testKit(first).spawnTestProbe("probe-one", expecting: CRDT.LWWMap<String, String?>.self)
let p2 = self.testKit(second).spawnTestProbe("probe-two", expecting: CRDT.LWWMap<String, String?>.self)

let one = try first.spawn("one", self.ownsLWWMap(p: p1, defaultValue: nil))
let two = try second.spawn("two", self.ownsLWWMap(p: p2, defaultValue: nil))

one.tell(.set(key: "a", value: "foo", .local))
one.tell(.set(key: "aa", value: nil, .local))

let gossipOneExpectMap: [String: String?] = ["a": .init("foo"), "aa": nil]
try self.expectMap(probe: p1, expected: gossipOneExpectMap)
try self.expectMap(probe: p2, expected: gossipOneExpectMap)

two.tell(.set(key: "b", value: "bar", .local))

let gossipTwoExpectMap: [String: String?] = ["a": "foo", "aa": nil, "b": "bar"]
try self.expectMap(probe: p1, expected: gossipTwoExpectMap)
try self.expectMap(probe: p2, expected: gossipTwoExpectMap)
}

func test_gossip_localLWWMapUpdate_toOtherNode() throws {
func test_gossip_localLWWMapUpdate_toOtherNode_codableValue() throws {
let configure: (inout ActorSystemSettings) -> Void = { settings in
settings.crdt.gossipInterval = .seconds(1)
settings.crdt.gossipIntervalRandomFactor = 0 // no random factor, exactly 1second intervals
Expand All @@ -140,24 +181,22 @@ final class CRDTGossipReplicationClusteredTests: ClusteredActorSystemsXCTestCase
let second = self.setUpNode("second", configure)
try self.joinNodes(node: first, with: second, ensureMembers: .up)

let p1 = self.testKit(first).spawnTestProbe("probe-one", expecting: CRDT.LWWMap<String, ValueHolder<String?>>.self)
let p2 = self.testKit(second).spawnTestProbe("probe-two", expecting: CRDT.LWWMap<String, ValueHolder<String?>>.self)
let p1 = self.testKit(first).spawnTestProbe("probe-one", expecting: CRDT.LWWMap<String, CodableExampleValue>.self)
let p2 = self.testKit(second).spawnTestProbe("probe-two", expecting: CRDT.LWWMap<String, CodableExampleValue>.self)

// TODO: JSON serialization blows up on Swift 5.2.4 Linux with top-level values so we must wrap (https://bugs.swift.org/browse/SR-13173). Change nilPlaceholder to `:String? = .none` once fixed
let nilPlaceholder = ValueHolder<String?>()
let one = try first.spawn("one", self.ownsLWWMap(p: p1, defaultValue: nilPlaceholder))
let two = try second.spawn("two", self.ownsLWWMap(p: p2, defaultValue: nilPlaceholder))
let one = try first.spawn("one", self.ownsLWWMap(p: p1, defaultValue: "<nil>"))
let two = try second.spawn("two", self.ownsLWWMap(p: p2, defaultValue: "<nil>"))

one.tell(.set(key: "a", value: .init("foo"), .local))
one.tell(.set(key: "aa", value: nilPlaceholder, .local))
one.tell(.set(key: "a", value: "foo", .local))
one.tell(.set(key: "aa", value: "baz", .local))

let gossipOneExpectMap: [String: ValueHolder<String?>] = ["a": .init("foo"), "aa": nilPlaceholder]
let gossipOneExpectMap: [String: CodableExampleValue] = ["a": "foo", "aa": "baz"]
try self.expectMap(probe: p1, expected: gossipOneExpectMap)
try self.expectMap(probe: p2, expected: gossipOneExpectMap)

two.tell(.set(key: "b", value: .init("bar"), .local))
two.tell(.set(key: "b", value: "bar", .local))

let gossipTwoExpectMap: [String: ValueHolder<String?>] = ["a": .init("foo"), "aa": nilPlaceholder, "b": .init("bar")]
let gossipTwoExpectMap: [String: CodableExampleValue] = ["a": "foo", "aa": "baz", "b": "bar"]
try self.expectMap(probe: p1, expected: gossipTwoExpectMap)
try self.expectMap(probe: p2, expected: gossipTwoExpectMap)
}
Copy link
Member Author

@ktoso ktoso Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would crash hard.

Expand Down Expand Up @@ -294,11 +333,11 @@ final class CRDTGossipReplicationClusteredTests: ClusteredActorSystemsXCTestCase
}
}

private func expectMap(probe: ActorTestProbe<CRDT.LWWMap<String, ValueHolder<String?>>>, expected: [String: ValueHolder<String?>], file: StaticString = #file, line: UInt = #line) throws {
private func expectMap<Value>(probe: ActorTestProbe<CRDT.LWWMap<String, Value>>, expected: [String: Value], file: StaticString = #file, line: UInt = #line) throws {
let testKit: ActorTestKit = self._testKits.first!

try testKit.eventually(within: .seconds(10)) {
let replicated: CRDT.LWWMap<String, ValueHolder<String?>> = try probe.expectMessage(within: .seconds(10), file: file, line: line)
let replicated: CRDT.LWWMap<String, Value> = try probe.expectMessage(within: .seconds(10), file: file, line: line)
pinfo("[\(probe.name)] received updated crdt: \(replicated)")

guard expected == replicated.underlying else {
Expand Down
10 changes: 4 additions & 6 deletions docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,10 @@ ENV LANG en_US.UTF-8
ENV LANGUAGE en_US.UTF-8

# dependencies
RUN apt-get update && apt-get install -y wget
RUN apt-get update && apt-get install -y lsof dnsutils netcat-openbsd net-tools curl jq # used by integration tests

# ruby and jazzy for docs generation
RUN apt-get update && apt-get install -y ruby ruby-dev libsqlite3-dev
RUN gem install jazzy --no-ri --no-rdoc
RUN apt-get update && apt-get install -y wget \
lsof dnsutils netcat-openbsd net-tools curl jq \
ruby ruby-dev libsqlite3-dev
RUN if [ "${ubuntu_version}" != "xenial" ] ; then gem install jazzy --no-ri --no-rdoc ; fi
RUN gem install asciidoctor -v 1.5.8 --no-ri --no-rdoc
RUN gem install asciidoctor-pdf --pre --no-ri --no-rdoc
RUN gem install asciidoctor-diagram -v 1.5.8 --no-ri --no-rdoc
Expand Down
2 changes: 1 addition & 1 deletion docker/docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ services:

test:
<<: *common
command: /bin/bash -cl "SACT_WARNINGS_AS_ERRORS=yes swift test --enable-test-discovery -Xswiftc -DSACT_TESTS_LEAKS && ./scripts/integration_tests.sh"
command: /bin/bash -cl "swift -version; SACT_WARNINGS_AS_ERRORS=yes swift test --enable-test-discovery -Xswiftc -DSACT_TESTS_LEAKS && ./scripts/integration_tests.sh"

bench:
<<: *common
Expand Down