Improve wait callback and timeout handling #110
Merged
Conversation
This file contains 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
All other errors are negative so that they can be returned from functions like read/write which return a positive number for success.
This allows handling and raising TimeoutError in the same way as our other errors. This also allows for us to report syscall errors which occur in the socket wait callback (which I believe are very unlikely).
When we call rb_wait_for_single_fd we release the GVL and allow our Ruby thread to receive interrupts. The most obvious case of this is using Timeout.timeout. If we see an exception in this way we can't safely use our socket anymore as it was likely in the middle of another operation, so it should be shut down.
composerinteralia
approved these changes
Aug 1, 2023
| return TRILOGY_SYSERR; | ||
| if (args.rc == 0) |
There was a problem hiding this comment.
I got confused for a second because in most other places rc == 0 means TRILOGY_OK. I understand now that this is the return value of rb_wait_for_single_fd, where 0 means a timeout: https://github.com/ruby/ruby/blob/1642e0c39220e95ddb16b4cbbbe78f24507dfd48/include/ruby/io.h#L902. Possibly worth an inline comment in addition to what you have in the commit message?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This makes a couple improvements to how we wait for the socket to become ready for I/O.
First this refactors
TRILOGY_RB_TIMEOUTto a "real" error status in the C library (though currently nothing in the C library raises it), which allows it to be negative like all the other errors (and avoids being confused with a successful read/write of size 1).Next this makes
_cb_ruby_waitreturn this new status code instead of SYSERR when there is a timeout, allowing it to propogate throughtrilogy_sock_upgrade_ssland similar correctly. This allows differentiating between actually syserrors which set errno and timeouts. Also this can show the difference betweenrb_wait_for_single_fdreturning-1(a syscall error, which I think in practice will be extremely rare) and 0 (a timeout).Finally the last two commits ensure that the socket is shut down on either a socket timeout we see, or from an external exception (like
Timeout.timeout). For the latter we must wrap the waiting in anrb_protect, which previously we were doing correctly for queries, but not for other operations (ex.ping,change_db). We have to shut down the socket because we've interrupted normal control flow and are likely either in the middle of a write (we've partly written our packet) or a read (there will be data sent from the server that we need to handle) so any further operations are invalid.