Implement Decoding of Messages Received from Backend #1

moritzsternemann · 2022-09-18T22:38:53Z

Description

Implement decoding of the most common messages that can be received from memcached via the Meta Protocol. More information can be found in the protocol reference.

The code still has a few TODOs. Some of them just need to be implemented and some are rather questions on the design. I'll comment on the latter ones individually. Happy about all feedback! 🙌

To be implemented before merging

Decode error responses
Decode flags
Decode Meta Debug and Meta No-op responses
More tests for the decoding
Tests for the ByteBuffer CRLF extension

moritzsternemann · 2022-09-18T22:39:39Z

Sources/Memcache/ChannelHandler/MemcacheBackendMessageDecoder.swift

+        do {
+            // Pass the buffer instead of messageSlice because .value messages continue after the first \r\n
+            let result = try MemcacheBackendMessage.decode(from: &buffer, for: verb)
+            // TODO: Can we make sure the message was read entirely? Difficult because we don't know the length of VA messages here.


Let me know if you have ideas!

Sources/Memcache/ChannelHandler/MemcacheDecodingError.swift

moritzsternemann · 2022-09-18T22:43:36Z

Sources/Memcache/ChannelHandler/Messages/Backend+Flags.swift

+
+extension MemcacheBackendMessage {
+    struct Flags: MemcacheMessagePayloadDecodable, Equatable, ExpressibleByArrayLiteral {
+        let flags: [String] // TODO: Do we want something like (Character, token: String?) instead? Or a struct?


If we try to parse the flags a bit further in this stage, we can catch malformed messages earlier and it might make parsing the meta-command-specific flags a bit easier. Flags are always a single character followed by an optional token string. Some tokens have a length limit but I don't think it makes sense to try to enforce it in this stage.

I think the flags should be an enum with associated values. And I think we should already enforce this here. I think users should be able to never decode anything after this stage and trust the values 100%.

Totally agree 👍 One potential issue however is that flags can have slightly different meanings depending on the sent command. For example, the N(token) flags is described as vivify on miss, takes TTL as argument for Meta Get, and as auto create item on miss with supplied TTL for Meta Arithmetic commands. I couldn't find any collisions so we just need to choose good names for the enum cases.

One thing we can't verify here though is which flags can be part of a message. This also depends on the sent command.

Edit: Or do you think we should just be unopinionated and directly use the flag characters as the enum case names?

I implemented decoding of flags, I think, as far as possible and added checks for things like data type and length where it makes sense.

Sources/Memcache/ChannelHandler/MemcacheDecodingError.swift

Sources/Memcache/Extensions/ByteBuffer+Memcache.swift

fabianfett · 2022-09-19T09:44:13Z

Sources/Memcache/ChannelHandler/MemcacheBackendMessageDecoder.swift

+        // Keep track of the reader index in case we later notice that we need more data
+        let startReaderIndex = buffer.readerIndex


Instead of handling indexes so much, a better way is normally to get a copy of the bytebuffer.

var peekableBuffer = buffer // if you were able to decode a buffer, just write the new reader indexes back: buffer = peakableBuffer

fabianfett · 2022-09-19T09:47:02Z

Sources/Memcache/ChannelHandler/MemcacheBackendMessageDecoder.swift

+        // Peek at the message to read the verb. It is before the first \r\n and before the first <space> if the message
+        // contains one.
+        guard let messageSlice = buffer.getCarriageReturnNewlineTerminatedSlice(at: buffer.readerIndex) else {
+            // reader index wasn't moved, wait for more bytes
+            return nil
+        }


What is the benefit of finding the first line, if that doesn't ensure that this will be the complete first message? IIUC you are interested in the first VERB and you can get that by finding either a space or a \r\n correct?

Yes correct. Though being able to read a line does not always indicate that we have a full message. For value messages (format: VA <data block size> <flags>\r\n<data block>\r\n), we only know how long the message should be when we start parsing the part after the verb.
If we would look for a space first, we might fail to parse the following buffer for example: EN\r\nHD <flags>\r\n.

Maybe the messageSlice name is also a bit misleading here 😇

Sources/Memcache/ChannelHandler/Messages/Backend+Value.swift

fabianfett

Great progress! Added some more comments...

fabianfett · 2022-09-27T06:39:27Z

Package.swift

    ],
    targets: [
        .target(
            name: "Memcache",
            dependencies: [
-                .product(name: "NIO", package: "swift-nio")
+                .product(name: "NIO", package: "swift-nio"),


Please explicitly import NIOCore and NIOPosix. Plain NIO shall go away with the next major release.

fabianfett · 2022-09-27T06:40:18Z

Package.swift

    ],
    targets: [
        .target(
            name: "Memcache",
            dependencies: [
-                .product(name: "NIO", package: "swift-nio")
+                .product(name: "NIO", package: "swift-nio"),
+                .product(name: "ExtrasBase64", package: "swift-extras-base64")


TBH. I would vendor in the decoding/encoding part that you need. Just make sure you mention that you vendor those parts. Also make them internal.

fabianfett · 2022-09-27T06:42:21Z

Sources/Memcache/Flags/MemcacheFlag.swift

+// MARK: -
+
+extension MemcacheFlag {
+    enum Code: Character {


I don't think we need Character here. I think using UInt8 likely gives us better performance, since we don't need to go through UTF8 validity checks.

fabianfett · 2022-09-27T06:45:11Z

Sources/Memcache/Flags/MemcacheFlags+Tokens.swift

+    struct NumericToken<Value: Numeric>: CustomDebugStringConvertible {
+        var value: Value
+
+        var debugDescription: String {
+            "numeric: \(value)"
+        }
+    }


What is the benefit of the NumericToken type? Why can't we use the Numeric values directly? Add this reasoning to the type...

fabianfett · 2022-09-27T06:45:24Z

Sources/Memcache/Flags/MemcacheFlags+Tokens.swift

+    struct StringToken: ExpressibleByStringLiteral, CustomDebugStringConvertible {
+        var value: String
+
+        init(stringLiteral value: String) {
+            self.value = value
+        }
+
+        var debugDescription: String {
+            "string: \(value)"
+        }
+    }


What do we need this wrapper for?

fabianfett · 2022-09-27T06:49:01Z

Sources/Memcache/Flags/MemcacheFlags+Tokens.swift

+
+    /// Mode switch token used in Set and Arithmetic commands.
+    enum ModeToken: RawRepresentable, CustomDebugStringConvertible {
+        typealias RawValue = Character


Make the RawValue = UInt8

fabianfett · 2022-09-27T06:49:25Z

Sources/Memcache/MemcacheBackendMessageDecoder.swift

@@ -0,0 +1,59 @@
+import Foundation


What do you need Foundation for here?

fabianfett · 2022-09-27T06:51:45Z

Sources/Memcache/MemcacheBackendMessageDecoder.swift

+
+        // Peek at the message to read the verb. It is before the first \r\n and before the first <space> if the message
+        // contains one.
+        guard let textLine = peekableBuffer.getCarriageReturnNewlineTerminatedSlice(at: peekableBuffer.readerIndex) else {


I would read here instead of getting. In the error cases you can pass the buffer that you keep unmodified for now.

Do you actually need to know the complete text line? Or is the goal really only to get the first verb to then learn how long the message will be?

fabianfett · 2022-09-27T06:54:37Z

Sources/Memcache/MemcacheBackendMessageDecoder.swift

+        let verbLength = (textLine.readableBytesView.firstIndex(of: .space) ?? textLine.writerIndex) - textLine.readerIndex
+
+        guard let verbString = textLine.getString(at: textLine.readerIndex, length: verbLength) else {
+            // If we can't read a string, the text line must be empty (i.e. no characters before the first occurence of \r\n)
+            throw MemcacheDecodingError.emptyMessageReceived(bytes: peekableBuffer)
+        }
+
+        guard let verb = MemcacheBackendMessage.Verb(rawValue: verbString) else {
+            throw MemcacheDecodingError.unknownVerbReceived(messageVerb: verbString, messageBytes: peekableBuffer)
+        }


I think I would put this into an extension on ByteBuffer, that I would call readVerb() throws -> MemcacheBackendMessage.Verb, which of course moves the readerIndex forward.

fabianfett · 2022-09-27T06:57:38Z

Sources/Memcache/MemcacheDecodingError.swift

+        line: UInt = #line
+    ) -> Self {
+        MemcacheDecodingError(
+            messageVerb: "",


I guess the messageVerb should be an optional?

FranzBusch

I think this looks really good already. Fabian left some good comments here and I added a few more. If we fix them up and then take another look I think we can make some good progress.

FranzBusch · 2022-11-01T15:17:06Z

Sources/Memcache/MemcacheDecodingError.swift

@@ -0,0 +1,53 @@
+import ExtrasBase64


Is this used here?

FranzBusch · 2022-11-01T15:19:09Z

Sources/Memcache/MemcacheDecodingError.swift

+import ExtrasBase64
+import NIOCore
+
+struct MemcacheDecodingError: Error {


We probably want to have a public error type at some point. I just left a comment over in the kafka repository with a common error pattern we have established. I think it would be great to adopt this here for the public error type that we use in the end. Might be done in a follow up PR but just leaving this here

FranzBusch · 2022-11-01T15:19:37Z

Sources/Memcache/Messages/Backend+ErrorMessage.swift

+            self.message = value
+        }
+
+        static func decode(from buffer: inout ByteBuffer) throws -> Self {


We should make this a method on ByteBuffer probably like readErrorMessage

FranzBusch · 2022-11-01T15:22:02Z

Sources/Memcache/Messages/Backend+Flags.swift

+        /// The following formats can be decoded from the `buffer`:
+        /// - `<flags>\r\n`. Flags are space-separated strings.
+        /// - `\r\n`. No flags.
+        static func decode(from buffer: inout ByteBuffer) throws -> Self {


FranzBusch · 2022-11-01T15:23:09Z

Sources/Memcache/Messages/Backend+Flags.swift

+                    .split(separator: " ")
+                    .map { flag in
+                        guard let codeCharacter = flag.first,
+                              let code = MemcacheFlag.Code(rawValue: codeCharacter)
+                        else {
+                            throw MemcachePartialDecodingError.fieldNotDecodable(as: MemcacheFlag.Code.self, from: String(flag))
+                        }
+                        return try .decode(from: flag.dropFirst(), for: code)
+                    }


I think we should look at that code complexity wise. It is iterating the flagsString multiple times from what I can see which we could avoid.

FranzBusch · 2022-11-01T15:23:34Z

Sources/Memcache/Messages/Backend+Value.swift

+        /// The message can have the following formats:
+        /// - `<size> <flags>\r\n<data block>\r\n`. Flags are space-separated strings.
+        /// - `<size>\r\n<data block>\r\n`
+        static func decode(from buffer: inout ByteBuffer) throws -> Self {


moritzsternemann added 4 commits September 17, 2022 12:43

Remove stubs

3df2fb9

Add MemcacheBackendMessage + tests

f6eebff

Add doc comments for messages

1b8357f

Implement decoding of most common messages + tests

1058a5b

moritzsternemann commented Sep 18, 2022

View reviewed changes

fabianfett reviewed Sep 19, 2022

View reviewed changes

Sources/Memcache/ChannelHandler/MemcacheDecodingError.swift Outdated Show resolved Hide resolved

fabianfett reviewed Sep 19, 2022

View reviewed changes

Sources/Memcache/ChannelHandler/MemcacheDecodingError.swift Outdated Show resolved Hide resolved

fabianfett reviewed Sep 19, 2022

View reviewed changes

Sources/Memcache/Extensions/ByteBuffer+Memcache.swift Outdated Show resolved Hide resolved

fabianfett reviewed Sep 19, 2022

View reviewed changes

Sources/Memcache/ChannelHandler/Messages/Backend+Value.swift Outdated Show resolved Hide resolved

moritzsternemann added 8 commits September 19, 2022 18:36

Use #fileID instead of #file for errors

e4ef67b

Remove unnecessary @inlinable

ec9769c

Add swift-extras-base64 for data printing

139909d

Decode flags

b37d880

Decode errors

b7502d1

Copy buffer to handle indices less

edc61fa

Add tests for error responses

80f2774

Move files out of ChannelHandler folder

5cc95b0

fabianfett reviewed Sep 27, 2022

View reviewed changes

FranzBusch reviewed Nov 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Decoding of Messages Received from Backend #1

Implement Decoding of Messages Received from Backend #1

moritzsternemann commented Sep 18, 2022 •

edited

Loading

moritzsternemann Sep 18, 2022

moritzsternemann Sep 18, 2022

fabianfett Sep 19, 2022

moritzsternemann Sep 19, 2022 •

edited

Loading

moritzsternemann Sep 26, 2022 •

edited

Loading

fabianfett Sep 19, 2022 •

edited

Loading

fabianfett Sep 19, 2022

moritzsternemann Sep 19, 2022 •

edited

Loading

fabianfett left a comment

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

fabianfett Sep 27, 2022

FranzBusch left a comment

FranzBusch Nov 1, 2022

FranzBusch Nov 1, 2022

FranzBusch Nov 1, 2022

FranzBusch Nov 1, 2022

FranzBusch Nov 1, 2022

FranzBusch Nov 1, 2022

		// Keep track of the reader index in case we later notice that we need more data
		let startReaderIndex = buffer.readerIndex

Implement Decoding of Messages Received from Backend #1

Are you sure you want to change the base?

Implement Decoding of Messages Received from Backend #1

Conversation

moritzsternemann commented Sep 18, 2022 • edited Loading

Description

To be implemented before merging

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moritzsternemann Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

moritzsternemann Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

fabianfett Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moritzsternemann Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

fabianfett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moritzsternemann commented Sep 18, 2022 •

edited

Loading

moritzsternemann Sep 19, 2022 •

edited

Loading

moritzsternemann Sep 26, 2022 •

edited

Loading

fabianfett Sep 19, 2022 •

edited

Loading

moritzsternemann Sep 19, 2022 •

edited

Loading