Created a decoder handler to deserialize a MemcachedResponse #11

dkz2 · 2023-05-26T10:18:17Z

Created a decoder handler that can transform a ByteBuffer received from the Memcached server into a MemcachedResponse. This PR will close #5.

Motivation:

This marks the beginning for future implementations of decoding a MemcachedResponse.

Modifications:

Implemented definition of a basic MemcachedResponse.
Implemented a basic decoder to deserialize a MemcachedResponse.
Added Unit and updated our Integration test.

Result:

We can now successfully decode a MemcachedResponse received from the Memcached server.

FranzBusch

Great start! I left some comments inline

FranzBusch · 2023-05-30T09:43:01Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+import NIOCore
+import NIOPosix
+
+struct MemcachedResponseDecoder: ByteToMessageDecoder {


Instead of using ByteToMessageDecoder can you use NIOSingleStepByteToMessageDecoder instead. This makes the implementation way easier normally.

FranzBusch · 2023-05-30T09:44:57Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+import NIOCore
+import NIOPosix
+
+struct MemcachedResponseDecoder: ByteToMessageDecoder {


Can we add this as a doc comment to this struct

Responses look like: <RC> <datalen*> <flag1> <flag2> <...>\r\n Where <RC> is a 2 character return code. The number of flags returned are based off of the flags supplied. <datalen> is only for responses with payloads, with the return code 'VA'. Flags are single character codes, ie 'q' or 'k' or 'I', which adjust the behavior of the command. If a flag requests a response flag (ie 't' for TTL remaining), it is returned in the same order as they were in the original command, though this is not strict. Flags are single character codes, ie 'q' or 'k' or 'O', which adjust the behavior of a command. Flags may contain token arguments, which come after the flag and before the next space or newline, ie 'Oopaque' or 'Kuserkey'. Flags can return new data or reflect information, in the same order they were supplied in the request. Sending an 't' flag with a get for an item with 20 seconds of TTL remaining, would return 't20' in the response. All commands accept a tokens 'P' and 'L' which are completely ignored. The arguments to 'P' and 'L' can be used as hints or path specifications to a proxy or router inbetween a client and a memcached daemon. For example, a client may prepend a "path" in the key itself: "mg /path/foo v" or in a proxy token: "mg foo Lpath/ v" - the proxy may then optionally remove or forward the token to a memcached daemon, which will ignore them. Syntax errors are handled the same as noted under 'Error strings' section below. For usage examples beyond basic syntax, please see the wiki: https://github.com/memcached/memcached/wiki/MetaCommands

FranzBusch · 2023-05-30T09:48:11Z

Sources/SwiftMemcache/Extensions/UInt16+ResponseStatus.swift

+
+typealias ResponseStatus = UInt16
+
+extension ResponseStatus {


Let's make this an enum called ReturnCode and nest it inside the MemcachedResponse type

FranzBusch · 2023-05-30T09:49:11Z

Sources/SwiftMemcache/Extensions/UInt16+ResponseStatus.swift

+    static let exists = generateCode(from: (.init(ascii: "E"), .init(ascii: "X")))
+    static let notFound = generateCode(from: (.init(ascii: "N"), .init(ascii: "F")))
+
+    init?(asciiValues: (UInt8, UInt8)) {


A couple of things here:

First let's take a UInt16 here instead and just do a switch over the values. We have 4 values that we want to match so we can just spell the out

Let's remove the ? from the init. We precondition if we don't know the return code

FranzBusch · 2023-05-30T09:54:47Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+            preconditionFailure("Response code could not be read.")
+        }
+
+        // Check if there's a whitespace character, this indicates flags are present


Not only flags but also datalen. Just invert the comment to say if there is not a whitespace then we are at the end of the line.

FranzBusch · 2023-05-30T09:56:10Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+        }
+
+        // Check if there's a whitespace character, this indicates flags are present
+        if buffer.readableBytes > 2, buffer.getInteger(at: buffer.readerIndex, as: UInt8.self) == UInt8.whitespace {


We should just read the next byte here and then base our decision of that. Also return needMoreData if there is not a single byte readable.

FranzBusch · 2023-05-30T09:56:39Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+            buffer.moveReaderIndex(forwardBy: 1)
+
+            // -2 for \r\n
+            _ = buffer.readSlice(length: buffer.readableBytes - 2)


We should precondition here that we really read \r\n

FranzBusch · 2023-05-30T09:58:42Z

Sources/SwiftMemcache/MemcachedResponse.swift

+
+import NIOCore
+
+enum MemcachedResponse {


I don't know if modelling this as an enum is correct. From the return codes we can't know what command was issued e.g. HD and EN is used by both get and set commands. Let's just model this out as a struct that has a returnCode. We are going to do the request-response matching in the handler in the end.

FranzBusch

We are getting closer. I just added a larger comment with how we have to change this around to make sure we can handle multiple responses and better handle partial responses in the buffer.

FranzBusch · 2023-05-31T08:54:39Z

Sources/SwiftMemcache/MemcachedResponse.swift

+//===----------------------------------------------------------------------===//
+
+struct MemcachedResponse {
+    enum ReturnCode: UInt16 {


We don't need to back this with the raw value right now

Suggested change

enum ReturnCode: UInt16 {

enum ReturnCode {

FranzBusch · 2023-05-31T08:54:50Z

Sources/SwiftMemcache/MemcachedResponse.swift

+        }
+    }
+
+    let returnCode: ReturnCode


Let's make this a var

Suggested change

let returnCode: ReturnCode

var returnCode: ReturnCode

FranzBusch · 2023-05-31T08:55:06Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+import NIOPosix
+
+struct MemcachedResponseDecoder: NIOSingleStepByteToMessageDecoder {
+    /// Responses look like:


Can we move this to the doc of the type. So two lines above

FranzBusch · 2023-05-31T08:59:23Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+        guard let firstReturnCode = buffer.readInteger(as: UInt8.self),
+              let secondReturnCode = buffer.readInteger(as: UInt8.self) else {


Can't we just read a single UInt16 here?

FranzBusch · 2023-05-31T09:00:15Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+        // Ensure the buffer has at least 3 bytes (minimum for a response code and newline)
+        guard buffer.readableBytes >= 3 else {
+            return nil // Need more data
+        }


I think we don't have to check this here we can just try to read and return nil further down.

FranzBusch · 2023-05-31T09:07:43Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+        // If there is not a whitespace, then we are at the end of the line.
+        guard buffer.readableBytes > 0, let nextByte = buffer.getInteger(at: buffer.readerIndex, as: UInt8.self) else {
+            return nil // Need more dat
+        }
+
+        if nextByte != UInt8.whitespace {
+            // We're at the end of the line
+            buffer.moveReaderIndex(forwardBy: 1)
+        } else {
+            // We have additional data or flags to read
+            buffer.moveReaderIndex(forwardBy: 1)
+
+            // Assert that we really read \r\n
+            guard buffer.readableBytes >= 2,
+                  buffer.getInteger(at: buffer.readerIndex, as: UInt8.self) == UInt8.carriageReturn,
+                  buffer.getInteger(at: buffer.readerIndex + 1, as: UInt8.self) == UInt8.newline else {
+                preconditionFailure("Response ending '\r\n' not found.")
+            }
+
+            buffer.moveReaderIndex(forwardBy: 2)
+        }


This part can be written better and checking that we also read the \s. Instead of checking for readable bytes we can always just try to read the next UInt8. Additionally we need to account for multiple flags so we should rather add a loop in here. Can you try to rewrite this using a loop where do the following logic

Read the next byte

Check if the byte is a whitespace

If whitespace then try to read a flag (we can stub out this code for now)

If not-whitespace then exit the loop

Try to read the next 3 bytes and check if all four bytes are now equal to \r\n

FranzBusch · 2023-05-31T09:28:43Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+    /// https://github.com/memcached/memcached/wiki/MetaCommands
+    typealias InboundOut = MemcachedResponse
+
+    func decode(buffer: inout ByteBuffer) throws -> InboundOut? {


This is almost right however you have to account for the fact that there are multiple responses inside the buffer. Furthermore, we are doing a bunch of duplicated work here if we get partial responses e.g. if we get everything except the \r\n we are going to re-parse the return code and flags every time. To avoid this we should introduce a little state machine to this decoder. Some rough code how this might look

private enum NextStep: Hashable { /// The initial step. case returnCode /// Decode the data length, flags or check if we are the end case dataLengthOrFlag(MemcachedResponse.ReturnCode) /// Decode the next flag case decodeNextFlag(MemcachedResponse.ReturnCode , UInt64, [Flag]) /// Decode end of line case decodeEndOfLine(MemcachedResponse.ReturnCode, UInt64?, [Flag]) etc... } private enum NextDecodeAction { /// We need more bytes to decode the next step. case waitForMoreBytes /// We can continue decoding. case continueDecodeLoop /// We have decoded the next response and need to return it. case returnDecodedResponse(MemcachedResponse) } /// The next step in decoding. private var nextStep: NextStep = .decodeReturnCode public mutating func decode(buffer: inout ByteBuffer) throws -> MemcachedResponse? { while self.nextStep != .none { switch try self.next(buffer: &buffer) { case .returnDecodedResponse(let response): return response case .waitForMoreBytes: return nil case .continueDecodeLoop: () } } return nil } private mutating func next(buffer: inout ByteBuffer) throws -> NextDecodeAction { switch self.nextStep { case .decodeReturnCode: // Try to decode the return code and move the state case .decodeDataLengthOrFlag(let returnCode): // Check if the return code is `VA` then decode the data length otherwise move state to flag decoding } } public mutating func decodeLast(buffer: inout ByteBuffer, seenEOF: Bool) throws -> MemcachedResponse? { // Try to decode what is left in the buffer. if let output = try self.decode(buffer: &buffer) { return output } guard buffer.readableBytes == 0 || seenEOF else { // If there are still readable bytes left and we haven't seen an EOF // then something is wrong with the message or how we called the decoder. throw SomEError } switch self.nextStep { // We have to decide what to do here. Some steps are fine to be in. } }

dkz2 · 2023-05-31T16:34:14Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+
+        case .decodeNextFlag(let returnCode, let dataLength, var flags):
+            if let nextByte = buffer.readInteger(as: UInt8.self), nextByte != UInt8.whitespace {
+                flags.append(nextByte)


this is going to allocate a new array each time, I could ignore flags for the time being

FranzBusch

This looks pretty good now. One last round of comments and then we are good to go!

FranzBusch · 2023-06-01T09:13:33Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+        /// Decode the next flag
+        case decodeNextFlag(MemcachedResponse.ReturnCode, UInt64?)
+        /// Decode end of line
+        case decodeEndOfLine(MemcachedResponse.ReturnCode, UInt64?)


Can we add a TODO here that we need to add a step for decoding the response data if the return code is VA

FranzBusch · 2023-06-01T09:15:48Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+            return .continueDecodeLoop
+
+        case .dataLengthOrFlag(let returnCode):
+            if returnCode == .stored {


This is not correct we should only decode the data length if the returnCode is VA

FranzBusch · 2023-06-01T09:18:15Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+                // TODO: Implement decoding of data length
+            }
+
+            // Check if the next bytes are \r\n


Instead of checking if the next two bytes are something we should rather read the next byte and just check if it is a whitespace. If is a whitespace we have to decode flags otherwise we check if it is a \ and continue to read the next 3 bytes. It is fine if we return waitForMoreBytes when we got a \ and don't have 3 more bytes in the buffer. This should be rarely hit and we don't have to introduce a new state.

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

FranzBusch · 2023-06-01T09:22:46Z

Tests/SwiftMemcacheTests/UnitTest/MemcachedResponseDecoderTests.swift

+    var decoder: MemcachedResponseDecoder!
+    var channel: EmbeddedChannel!


We don't need a Channel here we can just call the methods on the decoder without the channel.

FranzBusch · 2023-06-01T09:24:16Z

Tests/SwiftMemcacheTests/UnitTest/MemcachedResponseDecoderTests.swift

+        var buffer = ByteBufferAllocator().buffer(capacity: 8)
+        buffer.writeBytes(returnCode)
+        buffer.writeBytes([UInt8.carriageReturn, UInt8.newline])


Can we create a helper method called makeMemcachedResponseByteBuffer that takes a MemcachedResponse and encodes it for us. We will have a bunch more tests down the line and this will help us write them easily

FranzBusch · 2023-06-01T09:24:54Z

Tests/SwiftMemcacheTests/UnitTest/MemcachedResponseDecoderTests.swift

+        }
+    }
+
+    func testDecodeSetStoredResponse() throws {


Can we remove the Set from the method names since the responses don't indicate the request methods.

FranzBusch

So close! Last few minor comments!

FranzBusch · 2023-06-01T17:23:36Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+    /// processes the current state of the ByteBuffer.
+    enum NextStep: Hashable {
+        /// No further steps are needed, the decoding process is complete for the current message.
+        case none


I think we never transition to that state and since our state machine is looping we can get rid of this

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

FranzBusch · 2023-06-01T17:28:57Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+                self.nextStep = .returnCode
+                return .returnDecodedResponse(response)
+            } else {
+                preconditionFailure("Expected whitespace or \\r")


We should probably throw here instead of preconditioning since the input is from the network and this might be a denial of service vector otherwise.

FranzBusch · 2023-06-01T17:34:43Z

Tests/SwiftMemcacheTests/UnitTest/MemcachedResponseDecoderTests.swift

+
+        // If there's a data length, write it to the buffer.
+        if let dataLength = response.dataLength, response.returnCode == .VA {
+            buffer.writeInteger(dataLength, as: UInt64.self)


We are missing writing a whitespace here

FranzBusch

Good job!

created a MemcachedResponse decoder

2838923

dkz2 changed the title ~~created a MemcachedResponse decoder~~ Implement memcached response decoder and memcached response May 26, 2023

dkz2 changed the title ~~Implement memcached response decoder and memcached response~~ Created a decoder handler to deserialize a MemcachedResponse May 26, 2023

dkz2 mentioned this pull request May 26, 2023

Implement memcached response decoder and memcached response #10

Closed

dkz2 added 5 commits May 26, 2023 05:26

flags are parsed but not handled

4ce410d

soundness fix

c2e44ec

response status name cleanup

5624259

eof not needed

633e79b

accessor not needed

067a943

FranzBusch requested changes May 30, 2023

View reviewed changes

Response and decoder refactor

6ebbea2

FranzBusch requested changes May 31, 2023

View reviewed changes

dkz2 added 3 commits May 31, 2023 10:24

reimpl decoder as state machine

72b883e

decoder test clean up

abb43c8

no need to move reader in decodeEndOfLine

077d046

dkz2 commented May 31, 2023

View reviewed changes

dkz2 added 2 commits May 31, 2023 11:39

ignore flags

9a4f3be

consume flag but do nothing with it

7e385c3

FranzBusch requested changes Jun 1, 2023

View reviewed changes

revamp of decoder, removed un needed state

b7772ce

dkz2 requested a review from FranzBusch June 1, 2023 14:52

FranzBusch reviewed Jun 1, 2023

View reviewed changes

remove unused state, added fatal error

250f680

FranzBusch approved these changes Jun 1, 2023

View reviewed changes

FranzBusch merged commit 783ca54 into swift-server:main Jun 1, 2023

		guard let firstReturnCode = buffer.readInteger(as: UInt8.self),
		let secondReturnCode = buffer.readInteger(as: UInt8.self) else {

		var decoder: MemcachedResponseDecoder!
		var channel: EmbeddedChannel!

Created a decoder handler to deserialize a MemcachedResponse #11

Created a decoder handler to deserialize a MemcachedResponse #11

Uh oh!

Conversation

dkz2 commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FranzBusch 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!

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!

FranzBusch 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!

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!

FranzBusch 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!

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!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkz2 commented May 26, 2023 •

edited

Loading