Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow NIOEchoClient to receive fragmented echo responses. #2041

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

SeJV
Copy link
Contributor

@SeJV SeJV commented Feb 6, 2022

Allow NIOEchoClient to receive fragmented echo responses.

Motivation:

While sending a message to NIOEchoServer and receiving the echoed response, it is not guaranteed that the sent message arrives inside one channelRead() call. This would fail in the current implementation.

Modifications:

On channelRead the data will be written to a buffer until all sendBytes got received.

Result:

The updated NIOEchoClient can now buffer split InboundIn data and prints the result, when all sendBytes got received.

Tests:

Tests for executables require swift-tools-version 5.5. To recreate the tests of the new NIOEchoClient receive message argument from constructor instead of the readLine(). Additionally context.writeAndFlush() the received buffer instead of printing it.

let expectedMessage = "Expected to read"
let expectedBuffer = ByteBuffer(string: expectedMessage)

let handler = EmbeddedChannel(handler: EchoHandler(message: expectedMessage))
try handler.connect(to: .init(ipAddress: "127.0.0.1", port: 80), promise: nil)
// empty outbound buffer
_ = try handler.readOutbound(as: ByteBuffer.self)

// Test case: EchoHandler receives split buffer
try handler.writeInbound(ByteBuffer(string: String(expectedMessage.prefix(5))))
try handler.writeInbound(ByteBuffer(string: String(expectedMessage.dropFirst(5))))
XCTAssertEqual(try handler.readOutbound(as: ByteBuffer.self), expectedBuffer)

@swift-server-bot
Copy link

Can one of the admins verify this patch?

12 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice change, thanks!

@Lukasa
Copy link
Contributor

Lukasa commented Feb 7, 2022

@swift-nio-bot test this please

@Lukasa Lukasa enabled auto-merge (squash) February 7, 2022 09:08
@Lukasa Lukasa merged commit a4ad5eb into apple:main Feb 7, 2022
@SeJV SeJV deleted the EchoClientBufferInData branch February 7, 2022 09:51
@dnadoba dnadoba added the semver/none No version bump required. label Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants