-
Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-5815 support disabling socket timeouts #2185
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
connorsmacd
wants to merge
28
commits into
mongodb:master
Choose a base branch
from
connorsmacd:sockettimeoutms_0.CDRIVER-5815
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
2cf5adc
Add failing test
connorsmacd 6ed9323
Fix socketTimeoutMS=0 ignored in URI
connorsmacd 596727c
Treat 0 timeout as infinite
connorsmacd 5432608
Fix meaning of 0 timeout for TLS
connorsmacd bb23cbd
Revert "Fix socketTimeoutMS=0 ignored in URI"
connorsmacd 5678eec
Support socketTimeoutMS=inf
connorsmacd d4d1f45
Set SSL opts on test client
connorsmacd 4fb27cb
Remove include
connorsmacd 8c12d39
Merge branch 'master' into sockettimeoutms_0.CDRIVER-5815
connorsmacd d9811a7
Apply timeout changes to OpenSSL TLS
connorsmacd 969fe26
Handle case where immediate timeout is desired
connorsmacd 6e1a6db
Apply timeout channels to secure channel
connorsmacd 52c1672
Fix non-blocking timeout handling in TLS
connorsmacd 6d0b809
Fix handling of sub-millisecond timeouts
connorsmacd f8056fd
Refactor timeout handling to be less error-prone
connorsmacd 2a458ef
Rename test case
connorsmacd 2261cfd
Remove timing checks from test
connorsmacd 93b32a8
Refer to future fix ticket number
connorsmacd dfac42d
Warn if socketTimeoutMS=0 is specified
connorsmacd 3c59307
Add comment with ticket number
connorsmacd 7f4a63c
Document socketTimeoutMS=inf
connorsmacd 34f63a5
Document socketTimeoutMS=inf as a C Driver extension
connorsmacd 6ff501c
Use more familiar time point comparison syntax
connorsmacd f65bb76
Move new constants into private header
connorsmacd fb8f40f
Fix inconsistent integer widths
connorsmacd 1773b8a
Fix inconsistent east/west const
connorsmacd 5314d22
Move MONGOC_DEFAULT_SOCKETTIMEOUTMS back to mongoc-client.h
connorsmacd 4b7dce8
Fix fixed-width integer includes
connorsmacd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -36,10 +36,10 @@ struct _mongoc_stream_socket_t { | |||||
| static BSON_INLINE int64_t | ||||||
| get_expiration(int32_t timeout_msec) | ||||||
| { | ||||||
| if (timeout_msec < 0) { | ||||||
| return -1; | ||||||
| } else if (timeout_msec == 0) { | ||||||
| if (timeout_msec == MONGOC_SOCKET_TIMEOUT_IMMEDIATE) { | ||||||
| return 0; | ||||||
| } else if (timeout_msec <= 0) { | ||||||
| return -1; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Suggest drive-by documenting the meaning of |
||||||
| } else { | ||||||
| return (bson_get_monotonic_time() + ((int64_t)timeout_msec * 1000L)); | ||||||
| } | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect this may change how
0is interpreted when directly using themongoc_stream_tAPI. Themongoc_stream_tAPI is (unfortunately) public and has known users (e.g. userver here). Example:Suggest reverting to preserve the meaning of 0 as non-blocking in the
mongoc_stream_tAPI.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is quite unfortunate. While I could use the POSIX convention just for the public
mongoc_stream_tAPI, I do not think this is the best approach.Take
mongoc_stream_readwith OpenSSL enabled for example:mongoc_stream_readis called, we converttimeout_msecfrom the POSIX convention to the spec convention, then pass it tostream->readv.stream->readvis_mongoc_stream_tls_openssl_readvin this case, which eventually callsmongoc_stream_tls_openssl_bio_read.mongoc_stream_tls_openssl_bio_read, we callmongoc_stream_readagain, so we convert thetimeout_msecback to the POSIX convention.If we continue to mix conventions like this, then we are just asking for errors. In my opinion, it would be best to use the POSIX convention as much as possible internally and use the spec convention only where needed for spec compliance. In the case of
socketTimeoutMS, we can use the spec convention within the URI, but once we extract the timeout from the URI, we convert to the POSIX convention then stick with it. This would have the added benefit of being more intuitive and less error-prone when dealing with POSIX system calls that accept a timeout.That said, I recognize that being unaligned with the spec in this regard is less than ideal. This also goes against the recommendation in CDRIVER-5815:
Thoughts?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
mongoc_socket_tandmongoc_stream_tAPI have been an unfortunate source of codebase and behavior inconsistencies that is difficult to address due to being public API. They are also primarily responsible for blocking further int32 vs. int64 correctness improvements (CDRIVER-4596 and CDRIVER-4599) which were were unable to be scoped for the v2 release, as noted in #1475:Concerning "spec" vs. "POSIX" conventions, I am slightly in favor of limiting the POSIX convention to the
mongoc-stream-socket.ccomponent (whereget_expiration()converts from spec to POSIX) + implementing specific workarounds for the stream/socket API, since in most contexts except for the socket/stream API, we use the spec convention. If we want to use the POSIX convention throughout the codebase, I expect there will be comparatively more changes needed for that than what this PR currently proposes (e.g. consider also howmlib_timerAPI treats< 0as immediate timeout, whereas POSIX convention treats< 0as infinite timeout).