Skip to content

KTOR-9183 Forward original CancellationException in attachToUserJob#5581

Merged
osipxd merged 2 commits intoktorio:mainfrom
fru1tworld:fix/9183-timeout-cancellation
May 8, 2026
Merged

KTOR-9183 Forward original CancellationException in attachToUserJob#5581
osipxd merged 2 commits intoktorio:mainfrom
fru1tworld:fix/9183-timeout-cancellation

Conversation

@fru1tworld
Copy link
Copy Markdown
Contributor

@fru1tworld fru1tworld commented May 6, 2026

Subsystem
Client / Core

Motivation
KTOR-9183 — attachToUserJob's cleanup handler substitutes a generic CancellationException for the user job's cause, dropping TimeoutCancellationException. As a result withTimeout / withTimeoutOrNull wrapping a client call no longer recognises its own timeout.

Solution
Forward the original CancellationException (including subtypes) when the cause already is one; wrap into CancellationException(message) only for non-cancellation throwables.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026 •
edited
Loading

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ab864b56-5560-4e02-9802-d21edf13eada

📥 Commits

Reviewing files that changed from the base of the PR and between 31f2f98 and ac73c1c.

📒 Files selected for processing (2)
  • ktor-client/ktor-client-core/common/src/io/ktor/client/engine/Utils.kt
  • ktor-client/ktor-client-core/jvm/test/AttachToUserJobTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
  • ktor-client/ktor-client-core/common/src/io/ktor/client/engine/Utils.kt
📝 Walkthrough

Walkthrough

This PR improves how attachToUserJob handles job cancellation by preserving specific CancellationException subtypes. When a job completes with a CancellationException cause, the function now attempts to reuse that exact exception type rather than always constructing a generic new one, ensuring subtypes like TimeoutCancellationException are preserved when canceling callJob. A new test verifies this behavior.

Changes

Cancellation Exception Subtype Preservation

Layer / File(s) Summary
Core Implementation
ktor-client/ktor-client-core/common/src/io/ktor/client/engine/Utils.kt		 Adds explicit import for CancellationException and updates attachToUserJob to cast the completion cause to CancellationException when possible, otherwise construct a new one from cause.message.
Test Verification
ktor-client/ktor-client-core/jvm/test/AttachToUserJobTest.kt		 New test class verifies that attachToUserJob preserves TimeoutCancellationException subtype when cancellation originates from withTimeout.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: forwarding the original CancellationException in attachToUserJob, which aligns with the PR's primary objective.
Description check ✅ Passed The description follows the required template with all three sections (Subsystem, Motivation, Solution) completed with clear, relevant information about the issue and its fix.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

osipxd
osipxd requested changes May 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Overall it looks good I only have a couple of stylistic comments

Comment thread ktor-client/ktor-client-core/jvm/test/AttachToUserJobTimeoutTest.kt Outdated
Comment thread ktor-client/ktor-client-core/jvm/test/AttachToUserJobTimeoutTest.kt Outdated
Comment thread ktor-client/ktor-client-core/common/src/io/ktor/client/engine/Utils.kt Outdated
@fru1tworld
Copy link
Copy Markdown
Contributor Author

fru1tworld commented May 8, 2026

Thanks for the feedback! Fixed it!

@osipxd
Copy link
Copy Markdown
Member

osipxd commented May 8, 2026

Nicely done! 🏆

@osipxd osipxd merged commit 4766120 into ktorio:main May 8, 2026
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@osipxd osipxd osipxd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fru1tworld @osipxd