Skip to content

Implement read/write timeouts for BIO datagram #2610

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

smittals2
Copy link
Contributor

@smittals2 smittals2 commented Aug 13, 2025

Issues:

CryptoAlg-3287
CryptoAlg-3331

Description of changes:

Added

  • BIO_CTRL_DGRAM_SET_RECV_TIMEOUT
  • BIO_CTRL_DGRAM_GET_RECV_TIMEOUT
  • BIO_CTRL_DGRAM_SET_SEND_TIMEOUT
  • BIO_CTRL_DGRAM_GET_SEND_TIMEOUT

Testing:

  • Read timeout test added
  • For Write timeout, the APIs are tested but the behavior is not. Filling the send buffer to force a timeout with UDP is unreliable across different OS (buffers can be very large, or instead of timing out OS starts dropping packets).

Callouts:

  • We also change how we track error states for datagram sockets. Previously, on read/write errors, we would only store certain errors if they were considered "retryable" or non-fatal. In the case of timeouts, WSAETIMEDOUT on Windows is not considered retriable and therefore was never tracked in our error handling, making that code path unreachable. This PR changes this behavior to store the error value regardless of whether it is retriable, so we can always track the error state.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@smittals2 smittals2 changed the title Implement read/write timeouts for BIO datagram Implement get/set for read/write timeouts for BIO datagram Aug 13, 2025
@smittals2 smittals2 changed the title Implement get/set for read/write timeouts for BIO datagram Implement read/write timeouts for BIO datagram Aug 13, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@smittals2 smittals2 marked this pull request as ready for review August 13, 2025 21:06
@smittals2 smittals2 requested a review from a team as a code owner August 13, 2025 21:06
@smittals2 smittals2 requested a review from justsmth August 13, 2025 21:06
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2025

Codecov Report

❌ Patch coverage is 78.20513% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.71%. Comparing base (d6a3724) to head (743ef6b).
⚠️ Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
crypto/bio/dgram.c 76.56% 15 Missing ⚠️
crypto/bio/bio_socket_test.cc 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2610      +/-   ##
==========================================
- Coverage   78.72%   78.71%   -0.02%     
==========================================
  Files         645      645              
  Lines      110641   111156     +515     
  Branches    15646    15701      +55     
==========================================
+ Hits        87103    87494     +391     
- Misses      22840    22970     +130     
+ Partials      698      692       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 893 to 898
// Part 2: Test send timeout
// Triggering a send timeout requires filling the kernel's send buffer, which
// is unreliable. The OS may drop packets or have a very large buffer,
// preventing the send call from blocking. The API's get/set functionality
// is verified in the |SocketDatagramTimeouts| test.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing part 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its intentionally not there, i wasn't able to simulate forcing a write timeout. I'll get rid of the pt. 2 from the comments. In SocketDatagramTimeouts (the test above this one) we just do a simple get/set check to ensure API works correctly. But we are not able to test the behavior.

@smittals2 smittals2 requested a review from justsmth August 14, 2025 19:16
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.

3 participants