Skip to content

KTOR-9938: Apache5: Abort the exchange when data arrives for a cancelled body channel - #5906

Open
Ruslan Ibrahimau (IRus) wants to merge 1 commit into
ktorio:mainfrom
IRus:fix/apache5-unread-body
Open

Ruslan Ibrahimau (IRus) wants to merge 1 commit into
ktorio:mainfrom
IRus:fix/apache5-unread-body

Conversation

@IRus

Copy link
Copy Markdown
Contributor

When the execute block returned before the body arrived, the response channel was cancelled and consume() discarded incoming data without returning capacity. Apache then stopped reading, the stream never ended, and the consumer job never completed, so execute() hung. Cancelling the consumer job aborts the exchange through attachFuture.

Subsystem
Client, Apache5 engine

Motivation
https://youtrack.jetbrains.com/issue/KTOR-9938/Apache5-streaming-response-hangs-when-the-block-returns-before-the-body-arrives

Solution
Abort the request when nobody reads the body anymore, instead of hanging

…ed body channel

When the execute block returned before the body arrived, the response channel
was cancelled and consume() discarded incoming data without returning
capacity. Apache then stopped reading, the stream never ended, and the
consumer job never completed, so execute() hung. Cancelling the consumer job
aborts the exchange through attachFuture.

@osipxd Osip Fatkullin (osipxd) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the fix! It prompted me to investigate streaming response cancellation more closely, and I found several related issues, including KTOR-9943 and issues linked to it.

I'm requesting changes because this PR will need to be rebased after #5913 is merged.

class Apache5UnreadBodyTest {

@Test
fun `streaming response completes when the body arrives after the block returns`() = runTest(timeout = 10.seconds) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While reviewing this PR I managed to write a common test case that checks this behavior for all client engines. And after writing these tests I found some more cancellation-related problems affecting all client engines (KTOR-9943) 😓

I think we should merge #5913 first and after that you'll be able to remove this test and just remove except("Apache5") from cancelling rawContent stops the underlying stream test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#5913 merged into release/3.x

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can switch the base branch using ./switch-base-branch.sh script

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants