fix: Remove KeepAlive socket option#1671
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughRemoves the TCP keep-alive socket option assignment from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Testcontainers/Containers/ResourceReaper.cs (1)
307-308: Clean up the KeepAlive note (typo + commented-out code).Line 307 has a typo (“plateform”), and Line 308 leaves disabled code in place. Prefer a plain rationale comment (with issue reference) instead of commented-out executable code.
Suggested cleanup
- // On some plateform the connection fail with PlatformNotSupportedException: Sockets on this platform are invalid for use after a failed connection attempt. - // tcpClient.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true); + // KeepAlive is intentionally not set due to platform-specific failures after failed connect attempts. + // See: https://github.com/dotnet/runtime/issues/24917.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Testcontainers/Containers/ResourceReaper.cs` around lines 307 - 308, In ResourceReaper where the KeepAlive note appears, replace the typo-filled comment and the commented-out tcpClient.Client.SetSocketOption(...) line with a concise rationale: correct "plateform" to "platform", remove the disabled code, and add a short comment stating that KeepAlive calls were intentionally omitted because they can throw PlatformNotSupportedException on some platforms (include an internal issue/PR reference if needed); locate the comment near the TcpClient usage (the code that previously referenced tcpClient.Client.SetSocketOption) and update it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Testcontainers/Containers/ResourceReaper.cs`:
- Around line 307-308: In ResourceReaper where the KeepAlive note appears,
replace the typo-filled comment and the commented-out
tcpClient.Client.SetSocketOption(...) line with a concise rationale: correct
"plateform" to "platform", remove the disabled code, and add a short comment
stating that KeepAlive calls were intentionally omitted because they can throw
PlatformNotSupportedException on some platforms (include an internal issue/PR
reference if needed); locate the comment near the TcpClient usage (the code that
previously referenced tcpClient.Client.SetSocketOption) and update it
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71183c3f-273c-496a-a53c-63debf4fade4
📒 Files selected for processing (1)
src/Testcontainers/Containers/ResourceReaper.cs
524bade
into
testcontainers:develop
What does this PR do?
On some platform the TCPClient fail to connect because of KeepAlive settings.
Why is it important?
If the client fail to connect, TestContainer does not work at all.
Related issues
Relates dotnet/runtime#24917
Closes #1670
Summary by CodeRabbit