KTOR-9373 Make ConcurrentMap iteration safe on Native

[e5l]

Summary

• Fixes KTOR-9373
• On Native, ConcurrentMap.entries/keys/values returned live views of the underlying LinkedHashMap — the lock was only held during property access, not during iteration. Concurrent modification (e.g. remove from another thread) during iteration caused ConcurrentModificationException.
• Fix: return snapshot copies from entries/keys/values, matching JVM ConcurrentHashMap's weakly-consistent iteration semantics. Also synchronize size and isEmpty() which were reading without the lock.

Test plan

• Added test that exercises CIOEngine.close() with active endpoints (the original crash site)
• All existing tests in ktor-utils and ktor-client-cio continue to pass

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request adds a new test for concurrent GET requests during CIO engine shutdown and updates synchronization in ConcurrentMapNative to protect read operations (size, isEmpty, entries, keys, values) with locks rather than exposing direct delegate access.

Changes

Cohort / File(s)	Summary
Test Addition ktor-client/ktor-client-cio/common/test/CIOEngineTest.kt	New test method verifying that closing the engine with active endpoints does not throw ConcurrentModificationException when dispatching concurrent GET requests.
Synchronization Updates ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt	Size, isEmpty, and collection accessor methods (entries, keys, values) now execute under lock synchronization, returning copied collections instead of direct delegate references.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main change: making ConcurrentMap iteration safe on Native, with reference to the Ktor issue number.
Description check	✅ Passed	The description covers all required template sections (Subsystem, Motivation, Solution) with clear detail on the problem, rationale, and implementation approach, including test plan.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/KTOR-9373-cio-close-concurrent-modification

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt (1)

83-83: toString() reads delegate without holding the lock.

This could produce inconsistent output if another thread modifies the map concurrently. Consider synchronizing for consistency:

-    override fun toString(): String = "ConcurrentMapNative by $delegate"
+    override fun toString(): String = synchronized(lock) { "ConcurrentMapNative by $delegate" }

This is low risk since toString() is typically only used for debugging, but synchronizing would be consistent with the other accessors.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt` at line
83, toString() reads the shared property delegate without holding the map's
lock, risking inconsistent output; update ConcurrentMapNative.toString() to
acquire the same lock used by other accessors (the instance lock/Mutex used
throughout the class), capture the delegate (or a snapshot) inside that critical
section, and then build and return the string from that protected value so
toString() is consistent with other synchronized accesses.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt`:
- Line 83: toString() reads the shared property delegate without holding the
map's lock, risking inconsistent output; update ConcurrentMapNative.toString()
to acquire the same lock used by other accessors (the instance lock/Mutex used
throughout the class), capture the delegate (or a snapshot) inside that critical
section, and then build and return the string from that protected value so
toString() is consistent with other synchronized accesses.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8d2ca67c-84a7-4bd3-a77b-61800d32e039

📥 Commits

Reviewing files that changed from the base of the PR and between a5b2eb6 and 765246e.

📒 Files selected for processing (2)

• ktor-client/ktor-client-cio/common/test/CIOEngineTest.kt
• ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt

[e5l]

Addressed CI failure (iteration 1):

• Fixed ConcurrentModificationException in ConcurrentMap.entries on Native: The previous fix (LinkedHashSet(delegate.entries)) copied the set but the entry objects still held references to the original HashMap's EntryRef, causing checkForComodification to throw when accessing .value after the map was modified. Changed to LinkedHashMap(delegate).entries which creates a fully independent snapshot.

• CI result: The ContentTest.testEmptyContent failures on macosArm64 and mingwX64 (which were caused by ConcurrentModificationException in CIOEngine.close()) are now resolved. Remaining failures are unrelated flaky tests:

  • TCPSocketTest.testAcceptErrorOnImmediateSocketClose [linuxX64]
  • DarwinEngineTest.testRequestInRunBlockingDispatchersDefault [macosArm64]
  • HttpTimeoutTest.testSocketTimeoutWriteFailOnWrite [macosX64]
  • CurlHttp2Test [mingwX64] (pre-existing since build fix(deps): update dependency io.swagger.parser.v3:swagger-parser to v2.1.20 #3936)
  • ClientHeadersTest.testContentNegotiationMediaType [mingwX64]

[e5l]

CI failure analysis (iteration 2):

All 5 failing checks are unrelated to this PR's changes (ConcurrentMapNative.kt + CIOEngineTest.kt):

Native Windows X64 (3 failures):

• CurlHttp2Test.test protocol version is HTTP 2 — pre-existing since build fix(deps): update dependency io.swagger.parser.v3:swagger-parser to v2.1.20 #3936
• ClientHeadersTest.testContentNegotiationMediaType — CIO request timeout (flaky)
• HttpTimeoutTest.testGetRequestTimeoutWithSeparateReceive — Curl/CIO request timeout (flaky)

Native macOS Arm64 (2 failures):

• DarwinEngineTest.testRequestInRunBlockingDispatchersDefault — 1000ms timeout (flaky)
• DarwinLegacyEngineTest.testRequestInRunBlocking — 1000ms timeout (flaky)

Native macOS X64 (1 failure):

• HttpTimeoutTest.testSocketTimeoutWriteFailOnWrite — UncompletedCoroutinesError on Darwin engine (flaky)

Native Linux X64 (2 failures):

• TCPSocketTest.testAcceptErrorOnImmediateSocketClose — Expected IOException, got EINVAL PosixException (flaky)
• TestApplicationTest.testStreamingResponse — pre-existing since build #15172

Build All Core — composite failure from the above 4 native platform failures, plus additional JVM flaky tests (CacheLegacyStorageTest, CIOHttpRequestLifecycleTest, Netty/Jetty tests)

No actionable changes for this PR. The ConcurrentModificationException fix from iteration 1 resolved the test that was actually related to this PR's changes.