Skip to content

Commit

Permalink
Avoid delivering websocket frames twice on EOF.
Browse files Browse the repository at this point in the history
Motivation:

Unfortunately as a result of #108 the WebSocketFrameDecoder can emit a
WebSocketFrame more than once if the user closes the connection in
channelRead, or any other callback while decode() is on the call stack.
This is obviously less than ideal, as it can allow multiple delivery of
frames.

Modifications:

Given WebSocketFrameDecoder a no-op implementation of decodeLast.

Result:

No multi-delivery of frames.
  • Loading branch information
Lukasa committed Apr 27, 2018
1 parent ec47847 commit 99a8154
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 0 deletions.
5 changes: 5 additions & 0 deletions Sources/NIOWebSocket/WebSocketFrameDecoder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,11 @@ public final class WebSocketFrameDecoder: ByteToMessageDecoder {
return .needMoreData
}

public func decodeLast(ctx: ChannelHandlerContext, buffer: inout ByteBuffer) throws -> DecodingState {
// EOF is not semantic in WebSocket, so ignore this.
return .needMoreData
}

/// Apply a number of validations to the incremental state, ensuring that the frame we're
/// receiving is valid.
private func validateState() throws {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ extension WebSocketFrameDecoderTest {
("testDecoderRejectsFragmentedControlFrames", testDecoderRejectsFragmentedControlFrames),
("testDecoderRejectsMultibyteControlFrameLengths", testDecoderRejectsMultibyteControlFrameLengths),
("testIgnoresFurtherDataAfterRejectedFrame", testIgnoresFurtherDataAfterRejectedFrame),
("testClosingSynchronouslyOnChannelRead", testClosingSynchronouslyOnChannelRead),
]
}
}
Expand Down
45 changes: 45 additions & 0 deletions Tests/NIOWebSocketTests/WebSocketFrameDecoderTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,29 @@ private class CloseSwallower: ChannelOutboundHandler {
}
}

/// A class that calls ctx.close() when it receives a decoded websocket frame, and validates that it does
/// not receive two.
private final class SynchronousCloser: ChannelInboundHandler {
typealias InboundIn = WebSocketFrame

private var closeFrame: WebSocketFrame?

func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
let frame = self.unwrapInboundIn(data)
guard case .connectionClose = frame.opcode else {
ctx.fireChannelRead(data)
return
}

// Ok, connection close. Confirm we haven't seen one before.
XCTAssertNil(self.closeFrame)
self.closeFrame = frame

// Now we're going to call close.
ctx.close(promise: nil)
}
}

public class WebSocketFrameDecoderTest: XCTestCase {
public var decoderChannel: EmbeddedChannel!
public var encoderChannel: EmbeddedChannel!
Expand Down Expand Up @@ -285,4 +308,26 @@ public class WebSocketFrameDecoderTest: XCTestCase {
// Take the handler out for cleanliness.
XCTAssertNoThrow(try self.decoderChannel.pipeline.remove(handler: swallower).wait())
}

public func testClosingSynchronouslyOnChannelRead() throws {
// We're going to send a connectionClose frame and confirm we only see it once.
XCTAssertNoThrow(try self.decoderChannel.pipeline.add(handler: SynchronousCloser()).wait())

var errorCodeBuffer = self.encoderChannel.allocator.buffer(capacity: 4)
errorCodeBuffer.write(webSocketErrorCode: .normalClosure)
let frame = WebSocketFrame(fin: true, opcode: .connectionClose, data: errorCodeBuffer)

// Write the frame, send it through the decoder channel. We need to do this in one go to trigger
// a double-parse edge case.
self.encoderChannel.write(frame, promise: nil)
var frameBuffer = self.decoderChannel.allocator.buffer(capacity: 10)
while case .some(.byteBuffer(var d)) = self.encoderChannel.readOutbound() {
frameBuffer.write(buffer: &d)
}
XCTAssertNoThrow(try self.decoderChannel.writeInbound(frameBuffer))

// No data should have been sent or received.
XCTAssertNil(self.decoderChannel.readOutbound())
XCTAssertNil(self.decoderChannel.readInbound() as WebSocketFrame?)
}
}

0 comments on commit 99a8154

Please sign in to comment.