Implement a get request #14

dkz2 · 2023-06-07T03:51:35Z

Updated our Encoder and Decoder to serialize/deserialize a get MemcachedResponse. This pr closes #6.

Motivation:

This marks the beginning for being able to handle get operations, as well as the foundation set up for building a soon to come Memcached Actor.

Modifications:

Flag v for get command added.
Implemented serialization of get commands in our RequestEncoder.
Implemented extensions on ByteBuffer to allow us to read/write MemcachedFlags.
Implemented extension on ByteBuffer to allow us to read an integer from ASCII characters.
Condensed and Updated our ResponseDecoder decoding states.
Added Unit Test.

Result:
We can now successfully encode and decode a get MemcachedResponse that can be sent to the Memcached server

FranzBusch

Great start! Left some comments

FranzBusch · 2023-06-07T14:58:12Z

Sources/SwiftMemcache/MemcachedRequest.swift


+ struct GetCommand {
+ let key: String
+ var flags: [MemcachedFlag]


I think we want to use something more specialised than an array here. We probably should create a MemcachedFlags struct instead that holds all of the flags inline. We know what flags exists so we don't need the dynamism that array offers us.

FranzBusch · 2023-06-07T14:59:38Z

Sources/SwiftMemcache/MemcachedRequestEncoder.swift

+ out.writeBytes(command.key.utf8)
+
+ // write flags if there are any
+ for flag in command.flags {


Yeah we should create MemcachedFlags struct and create a separate extension method on ByteBuffer that offers a mutating func write(flags: MemcachedFlags)

FranzBusch · 2023-06-07T15:00:31Z

Sources/SwiftMemcache/MemcachedResponse.swift


 var returnCode: ReturnCode
 var dataLength: UInt64?
+ var flags: [MemcachedFlag]?


Same here let's not use an array and lets add a mutating func readMemcachedFlags() -> MemcachedFlags to ByteBuffer

FranzBusch · 2023-06-07T15:01:47Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

 self.nextStep = .dataLengthOrFlag(returnCode)
 return .continueDecodeLoop

 case .dataLengthOrFlag(let returnCode):


I am wondering if we should separate this into two steps one dataLength and one flag and we can decide what the next step is in the case above when we decoded the return code.

FranzBusch · 2023-06-07T15:03:13Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

 if returnCode == .VA {
- // TODO: Implement decoding of data length
- fatalError("Decoding for VA return code is not yet implemented")
+ guard let dataLength = buffer.readInteger(as: UInt64.self) else {


Is this correct? Is the data length encoded as a UInt64 or rather written out as text?

you are correct, it is written out as text, so I will make that change as well!

FranzBusch · 2023-06-07T15:45:50Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

 return .returnDecodedResponse(response)
+
+ case .decodeValue(let returnCode, let dataLength, let flags):
+ guard buffer.readableBytes >= dataLength else {


We should probably check here for dataLength + carriageReturn + newLine. To make sure we are not decoding the data until we also have the returns

FranzBusch · 2023-06-07T15:48:28Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ var data: ByteBuffer = .init()
+ for _ in 0..<dataLength {
+ let byte = buffer.readInteger(as: UInt8.self)
+ data.writeInteger(byte!)


There is a method to read a slice of a buffer which we should use here readSlice(length:)

FranzBusch · 2023-06-07T15:49:13Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ nextByte == UInt8.carriageReturn,
+ let nextNextByte = buffer.readInteger(as: UInt8.self),
+ nextNextByte == UInt8.newline else {
+ return .waitForMoreBytes


When we change the logic here to guard that we have these bytes readable as well we should probably fatalError here because it should never happen

FranzBusch · 2023-06-14T12:35:03Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ ///
+ /// - parameters:
+ /// - integer: The MemcachedFlag to serialize.
+ mutating func write(flags: MemcachedFlags) {


Naming NIT:

Suggested change

mutating func write(flags: MemcachedFlags) {

mutating func writeMemcachedFlags(_ flags: MemcachedFlags) {

FranzBusch · 2023-06-14T12:35:14Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ /// If a flag is set, its corresponding byte value and a whitespace character is written into the ByteBuffer.
+ ///
+ /// - parameters:
+ /// - integer: The MemcachedFlag to serialize.


That doc is outdated

FranzBusch · 2023-06-14T12:35:54Z

Sources/SwiftMemcache/MemcachedFlags.swift

+//===----------------------------------------------------------------------===//
+
+struct MemcachedFlags {
+ var v: Bool = false


Can we give this a good name of what it does and add a doc comment?

This should also probably be optional since all flags are optional.

FranzBusch · 2023-06-14T12:36:37Z

Sources/SwiftMemcache/MemcachedFlags.swift

+extension MemcachedFlags: Equatable {
+ static func == (lhs: MemcachedFlags, rhs: MemcachedFlags) -> Bool {
+ return lhs.v == rhs.v
+ }
+}
+
+extension MemcachedFlags: Hashable {
+ func hash(into hasher: inout Hasher) {
+ hasher.combine(v)
+ }
+}


You don't have to implement those. The compiler will synthesise them for you if you write

extension MemcachedFlags: Hashable {}

great catch, didnt think about that!

FranzBusch · 2023-06-14T12:37:42Z

Sources/SwiftMemcache/MemcachedFlags.swift

+ var bytes: Set<UInt8> {
+ var result = Set<UInt8>()
+ for (keyPath, byte) in Self.flagToByte {
+ if self[keyPath: keyPath] {
+ result.insert(byte)
+ }
+ }
+ return result
+ }


We shouldn't allocate the intermediate Set here. We can do all of this in the writeMemcachedFlags method. Just go over each field of the struct, check if it is not-nil and write it out.

FranzBusch · 2023-06-26T08:44:41Z

Sources/SwiftMemcache/Extensions/UInt8+Characters.swift

+extension UInt8 {
+ static var size: Int { MemoryLayout<UInt8>.size }


Are we using this anywhere?

correct no longer needed!

FranzBusch · 2023-06-26T08:47:02Z

Sources/SwiftMemcache/MemcachedFlags.swift

+ static let flagToByte: [KeyPath<MemcachedFlags, Bool?>: UInt8] = [
+ \MemcachedFlags.shouldReturnValue: 0x76,
+ ]


I don't think this is going to work for all flags. Some of the flags carry actual value e.g. a numeric TTL. I think we really just have to spell this out in the write method. Like just go field by field of the struct and check if it is optional or not.

FranzBusch · 2023-06-26T08:47:18Z

Sources/SwiftMemcache/MemcachedFlags.swift

+ }
+}
+
+extension MemcachedFlags: Equatable {}


You can remove this since Hashable implies Equatable

Suggested change

extension MemcachedFlags: Equatable {}

FranzBusch · 2023-06-26T08:50:58Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ ///
+ /// - returns: An instance of `MemcachedFlags` containing the flags read from the buffer.
+ mutating func readMemcachedFlags() -> MemcachedFlags {
+ var flagBytes: Set<UInt8> = []


Allocating this set is unnecessary. I would rather init an "empty" MemcachedFlags in the beginning and then read byte by byte. For now we drop everything that is not a v flag.

FranzBusch · 2023-06-26T08:56:08Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ /// - returns: An instance of `MemcachedFlags` containing the flags read from the buffer.
+ mutating func readMemcachedFlags() -> MemcachedFlags {
+ var flagBytes: Set<UInt8> = []
+ while let nextByte = self.readInteger(as: UInt8.self), nextByte != UInt8.whitespace {


I don't think this is correct. The next byte can be a whitespace since flags are separated by whitespaces. Furthermore, the next byte could be a newline + carriage return. We probably have to do this differently and handle it inside the decoder itself because we need to understand the state of the whole response, e.g. it depends what kind of response we are decoding if there is anything following the flags or not.

FranzBusch · 2023-06-26T08:56:39Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

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


Why do we need two different states here?

FranzBusch · 2023-06-26T08:58:07Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift


 let returnCode = MemcachedResponse.ReturnCode(bytes)
- self.nextStep = .dataLengthOrFlag(returnCode)
+ self.nextStep = .dataLength(returnCode)


We should check the return code here and decide if we transition to dataLength or flags

FranzBusch · 2023-06-26T09:03:41Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ case .flag(let returnCode, let dataLength):
+ let flags = buffer.readMemcachedFlags()
+
+ guard buffer.readableBytes >= 2,
+ let nextByte = buffer.readInteger(as: UInt8.self),
+ nextByte == UInt8.carriageReturn,
+ let nextNextByte = buffer.readInteger(as: UInt8.self),
+ nextNextByte == UInt8.newline else {
+ preconditionFailure("Expected to find CRLF at end of response")
 }

+ self.nextStep = .decodeValue(returnCode, dataLength!, flags)
+ return .continueDecodeLoop
+
 case .decodeNextFlag(let returnCode, let dataLength):
- while let nextByte = buffer.readInteger(as: UInt8.self), nextByte != UInt8.whitespace {
- // for now consume the byte and do nothing with it.
- // TODO: Implement decoding of flags
+ let flags = buffer.readMemcachedFlags()


I think we can combine these two cases

FranzBusch · 2023-07-03T09:42:58Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ mutating func readMemcachedFlags() -> MemcachedFlags {
+ var flags = MemcachedFlags()
+ while let nextByte = self.readInteger(as: UInt8.self) {
+ switch nextByte {
+ case UInt8.v:
+ flags.shouldReturnValue = true
+ case UInt8.whitespace:
+ continue
+ case UInt8.carriageReturn:
+ guard let _ = self.readInteger(as: UInt8.self), self.readableBytes > 0 else {
+ break
+ }
+ default:
+ preconditionFailure("Unrecognized flag.")
+ }
+ }
+ return flags
+ }


I still see a couple of problems here:

We have to verify after the carriage return that we get a newline

More importantly it might be that our buffer doesn't contain the full flags yet. We have to support incremental parsing of flags. After thinking about this a bit, we should probably move the flag reading out of this method and into the state machine. We should be trying to read an individual flag + its token and stay in the decodeFlag state until we are done. We should also write a test for this where we only have a buffer that contains let's say the flag but not the carriage return and newline and send that as a second buffer into the decode method.

I just added a comment further down that basically says we should try to not do 2 yet since it makes everything a bit more complicated. However can you please add a test where you feed a partial buffer in that doesn't have everything i.e. only a \r and not the \n afterwards and then pass it in after and assert the right things happen.

This will be handled in a future PR!

FranzBusch · 2023-07-03T09:43:11Z

Sources/SwiftMemcache/MemcachedFlags.swift

+
+ init() {}
+
+ init(flagBytes: Set<UInt8>) {


Do we still need this?

FranzBusch · 2023-07-03T09:43:59Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ /// Decode the data length
+ case dataLength(MemcachedResponse.ReturnCode)
+ /// Decode the flags
+ case flags(MemcachedResponse.ReturnCode, UInt64?)


let's rename this to decodeFlag and always try to decode a single flag at a time. We just have to update the flags struct and store it as a state here.

Suggested change

case flags(MemcachedResponse.ReturnCode, UInt64?)

case flags(MemcachedResponse.ReturnCode, UInt64?)

FranzBusch · 2023-07-03T09:44:31Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ /// Decode the flags
+ case flags(MemcachedResponse.ReturnCode, UInt64?)
+ /// Decode the Value
+ case decodeValue(MemcachedResponse.ReturnCode, UInt64, MemcachedFlags?)


We don't need an optional for flags here we can just always have it since all its field are optional

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

FranzBusch · 2023-07-03T09:50:55Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ while let currentByte = buffer.getInteger(at: buffer.readerIndex, as: UInt8.self), currentByte == UInt8.whitespace {
+ buffer.moveReaderIndex(forwardBy: 1)
+ }


How can there be more than 1 whitespace? Shouldn't we rather just read the next byte and make sure it is a whitespace. Then afterwards we check where the next whitespace is by just getInteger forward. Once we have confirmed that we have a whitespace in the buffer we try to reach the integer from ASCII.

FranzBusch · 2023-07-03T09:58:24Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ // Skip over the whitespace, newline or carriage return
+ while let currentByte = buffer.getInteger(at: buffer.readerIndex, as: UInt8.self),
+ [UInt8.whitespace, UInt8.newline, UInt8.carriageReturn].contains(currentByte) {
+ buffer.moveReaderIndex(forwardBy: 1)
+ }
+
+ let isNextByteWhitespace = buffer.getInteger(at: buffer.readerIndex) == UInt8.whitespace
+ self.nextStep = isNextByteWhitespace ? .flags(returnCode, dataLength) : .decodeValue(returnCode, dataLength, nil)


I think we shouldn't do this here because we actually might have a short buffer. I would recommend that after reading the ASCII integer to transition to the next state which is decodeFlags. In that state we have to handle this whitespace/carriageReturn&newline logic.

FranzBusch · 2023-07-03T10:02:03Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

- if nextByte == UInt8.whitespace {
- self.nextStep = .decodeNextFlag(returnCode, nil)
+ case .flags(let returnCode, let dataLength):
+ let flags = buffer.readMemcachedFlags()


So this is where it gets a bit tricky and I would like us to take a shortcut for now. The problem is that we can have a partial buffer where we don't have the carriageReturn&newline yet and we would need to support reading partial flags. This also includes reading the partial tokens of flags which might be quite big. In a best world, we would go flag by flag and move ourselves forward. However, implementing this requires a bit of logic. What I would do for now is trying to scan the buffer forward to see if we have a \r\n in the buffer. If we do we are going to try and decode if not we are going to return needMoreData and let's create an issue for us to revisit this in the future.

Sounds good, we will create a new issue to handle the partial buffer implementation.

FranzBusch · 2023-07-03T10:07:28Z

Tests/SwiftMemcacheTests/UnitTest/MemcachedResponseDecoderTests.swift

 try self.testDecodeResponse(buffer: &buffer, expectedReturnCode: .NF)
 }
+
+ func testDecodeValueResponse() throws {


Yeah we need a couple of more tests here that are trying to pass partial responses and assert that we return the right things.

FranzBusch · 2023-07-06T07:46:24Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ // We were expecting a newline after the carriage return, but didn't get it.
+ return flags


I think we can fatalError here for now. This shouldn't happen

FranzBusch · 2023-07-06T07:47:24Z

Sources/SwiftMemcache/MemcachedResponseDecoder.swift

+ guard buffer.readableBytes >= 2,
+ let lastTwoBytes = buffer.getBytes(at: buffer.writerIndex - 2, length: 2),
+ lastTwoBytes[0] == UInt8.carriageReturn,
+ lastTwoBytes[1] == UInt8.newline else {
+ return .waitForMoreBytes
+ }


That check isn't correct. Instead of checking the last bytes we have to walk the buffer to find the first occurrence of \r\n since there might be more than one response in the buffer.

modified this to find the first occurrence!

FranzBusch · 2023-07-06T07:48:46Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ // If it wasn't a newline, it could be the start of the data block.
+ return flags


I think we need to fatalError if it is not a newline anything else is unexpected.

FranzBusch · 2023-07-06T07:50:19Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+ // Encountered a character we weren't expecting. This could be the start of the data block.
+ return flags


We are not parsing the v flag anymore here. Additionally let's fatalError here as well for now since we don't support the other flags and the comment is wrong. It cannot be the start of the data block

FranzBusch

LGTM! Let's get this merged so that we can focus on the actual Connection actor

init get request structure

87d1682

FranzBusch requested changes Jun 7, 2023

View reviewed changes

re work MemcachedFlags model

186088c

FranzBusch reviewed Jun 14, 2023

View reviewed changes

dkz2 added 2 commits June 14, 2023 21:34

dataLengthOrFlag split + readMemcachedFlags

d772994

decoder pr refactors

1bdd64d

FranzBusch reviewed Jun 26, 2023

View reviewed changes

dkz2 added 6 commits June 27, 2023 06:34

write and read memcached flag fixes

e3a764c

combine flag states into one

c2e774f

data length is written as text not encoded as UInt64

bafe676

remaining PR changes

923cf2b

optimized dataLength read by fixing readIntegerFromASCII

423d8f9

removed flagToByte prop

48772a6

dkz2 marked this pull request as ready for review June 30, 2023 12:43

final clean up closes swift-server#6

79b653f

FranzBusch reviewed Jul 3, 2023

View reviewed changes

dkz2 added 5 commits July 5, 2023 23:13

flag state clean up + set up for partial buffers impl to come soon

9056f40

closes swift-server#6

118b029

ensure we have \r\n before parsing

6bad1ff

\r\n + partial/split buffer test

437783a

closes swift-server#6

bfb446a

dkz2 requested a review from FranzBusch July 6, 2023 06:12

.suffix(2) is costly removed it

bc1b03b

FranzBusch reviewed Jul 6, 2023

View reviewed changes

walk buffer to find first \r\n

4288843

FranzBusch approved these changes Jul 6, 2023

View reviewed changes

FranzBusch merged commit ed88f85 into swift-server:main Jul 6, 2023

	mutating func write(flags: MemcachedFlags) {
	mutating func writeMemcachedFlags(_ flags: MemcachedFlags) {

		extension UInt8 {
		static var size: Int { MemoryLayout<UInt8>.size }

	case flags(MemcachedResponse.ReturnCode, UInt64?)
	case flags(MemcachedResponse.ReturnCode, UInt64?)

		// We were expecting a newline after the carriage return, but didn't get it.
		return flags

		// If it wasn't a newline, it could be the start of the data block.
		return flags

		// Encountered a character we weren't expecting. This could be the start of the data block.
		return flags

Implement a get request #14

Implement a get request #14

Uh oh!

Conversation

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

Uh oh!

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

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

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

Choose a reason for hiding this comment

Uh oh!

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

Labels

2 participants

dkz2 commented Jun 7, 2023 •

edited

Loading