Skip to content
Merged
Prev Previous commit
Next Next commit
dataLengthOrFlag split + readMemcachedFlags
  • Loading branch information
dkz2 committed Jun 15, 2023
commit d7729940b72e0a383b9dd429a40c5786e0050320
13 changes: 13 additions & 0 deletions Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,16 @@ extension ByteBuffer {
}
}
}

extension ByteBuffer {
/// Read flags from this `ByteBuffer`, moving the reader index forward appropriately.
///
/// - returns: An instance of `MemcachedFlags` containing the flags read from the buffer.
mutating func readMemcachedFlags() -> MemcachedFlags {
var flagBytes: Set<UInt8> = []
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

while let nextByte = self.readInteger(as: UInt8.self), nextByte != UInt8.whitespace {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

flagBytes.insert(nextByte)
}
return MemcachedFlags(flagBytes: flagBytes)
}
}
2 changes: 1 addition & 1 deletion Sources/SwiftMemcache/MemcachedFlags.swift
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,6 @@ extension MemcachedFlags: Equatable {

extension MemcachedFlags: Hashable {
func hash(into hasher: inout Hasher) {
hasher.combine(v)
hasher.combine(self.v)
}
}
69 changes: 33 additions & 36 deletions Sources/SwiftMemcache/MemcachedResponseDecoder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ struct MemcachedResponseDecoder: NIOSingleStepByteToMessageDecoder {
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 data length
case dataLength(MemcachedResponse.ReturnCode)
/// Decode the flag
case flag(MemcachedResponse.ReturnCode, UInt64?)
/// Decode the next flag
case decodeNextFlag(MemcachedResponse.ReturnCode, UInt64?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need two different states here?

// TODO: Add a next step for decoding the response data if the return code is VA
Expand Down Expand Up @@ -114,58 +116,53 @@ struct MemcachedResponseDecoder: NIOSingleStepByteToMessageDecoder {
}

let returnCode = MemcachedResponse.ReturnCode(bytes)
self.nextStep = .dataLengthOrFlag(returnCode)
self.nextStep = .dataLength(returnCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return .continueDecodeLoop

case .dataLengthOrFlag(let returnCode):
case .dataLength(let returnCode):
if returnCode == .VA {
guard let dataLength = buffer.readInteger(as: UInt64.self) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return .waitForMoreBytes
}

var flagBytes: Set<UInt8> = []
while let nextByte = buffer.readInteger(as: UInt8.self), nextByte != UInt8.whitespace {
flagBytes.insert(nextByte)
}

let flags = MemcachedFlags(flagBytes: flagBytes)

guard let nextByte = buffer.readInteger(as: UInt8.self),
nextByte == UInt8.carriageReturn,
let nextNextByte = buffer.readInteger(as: UInt8.self),
nextNextByte == UInt8.newline else {
self.nextStep = .flag(returnCode, dataLength)
return .continueDecodeLoop
} else {
guard let nextByte = buffer.readInteger(as: UInt8.self) else {
return .waitForMoreBytes
}

self.nextStep = .decodeValue(returnCode, dataLength, flags)
return .continueDecodeLoop
if nextByte == UInt8.whitespace {
self.nextStep = .decodeNextFlag(returnCode, nil)
return .continueDecodeLoop
} else if nextByte == UInt8.carriageReturn {
guard let nextNextByte = buffer.readInteger(as: UInt8.self), nextNextByte == UInt8.newline else {
return .waitForMoreBytes
}
let response = MemcachedResponse(returnCode: returnCode, dataLength: nil)
self.nextStep = .returnCode
return .returnDecodedResponse(response)
} else {
throw MemcachedDecoderError.unexpectedCharacter(nextByte)
}
}

guard let nextByte = buffer.readInteger(as: UInt8.self) else {
case .flag(let returnCode, let dataLength):
let flags = buffer.readMemcachedFlags()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

if nextByte == UInt8.whitespace {
self.nextStep = .decodeNextFlag(returnCode, nil)
return .continueDecodeLoop
} else if nextByte == UInt8.carriageReturn {
guard let nextNextByte = buffer.readInteger(as: UInt8.self), nextNextByte == UInt8.newline else {
return .waitForMoreBytes
}
let response = MemcachedResponse(returnCode: returnCode, dataLength: nil)
self.nextStep = .returnCode
return .returnDecodedResponse(response)
} else {
throw MemcachedDecoderError.unexpectedCharacter(nextByte)
}
self.nextStep = .decodeValue(returnCode, dataLength!, flags)
return .continueDecodeLoop

case .decodeNextFlag(let returnCode, let dataLength):
var flagBytes: Set<UInt8> = []
while let nextByte = buffer.readInteger(as: UInt8.self), nextByte != UInt8.whitespace {
flagBytes.insert(nextByte)
}
let flags = buffer.readMemcachedFlags()

let flags = MemcachedFlags(flagBytes: flagBytes)
let response = MemcachedResponse(returnCode: returnCode, dataLength: dataLength, flags: flags)
self.nextStep = .returnCode
return .returnDecodedResponse(response)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,3 @@ final class MemcachedFlagsTests: XCTestCase {
XCTAssertTrue(flags.v)
}
}