[Compatibility] Adding IO#timeout #3066
Open
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
oracle-contributor-agreement
bot
added
the
OCA Verified
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
af77b4a
to
f9612a2
Compare
eregon
reviewed
May 26, 2023
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
5 times, most recently
from
6d60df1
to
e4165b9
Compare
eregon
reviewed
May 29, 2023
There was a problem hiding this comment.
This looks fine. My main concern is we shouldn't make the no-timeout-set case slower, but that seems already fine here.
I wonder if it should only affect read/write though, or other operations as well?
Also could you untag the CRuby tests for this: test/ruby/test_io_timeout.rb?
eregon
reviewed
May 29, 2023
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
2 times, most recently
from
490dfbb
to
ab4a579
Compare
itarato
changed the title
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
2 times, most recently
from
759945e
to
cb1fa10
Compare
eregon
reviewed
Jun 6, 2023
src/main/ruby/truffleruby/core/io.rb
Outdated
| end | ||
|
|
||
| def timeout=(new_timeout) | ||
| self.nonblock = !Primitive.nil?(new_timeout) |
There was a problem hiding this comment.
Does io.timeout = nil sets nonblock to false?
Could you add specs for the interaction of timeout and nonblock?
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
cb1fa10
to
92c516a
Compare
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
92c516a
to
eb27291
Compare
andrykonchin
reviewed
Jun 7, 2023
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
eb27291
to
607745e
Compare
andrykonchin
reviewed
Jun 8, 2023
| raise(TypeError, 'time interval must not be negative') if seconds < 0 | ||
|
|
||
| seconds | ||
| end |
There was a problem hiding this comment.
Nice, it's a proper place for this helper method 👍.
|
|
||
| poll_result = Truffle::IOOperations.poll(io, Truffle::IOOperations::POLLOUT, current_timeout) | ||
| raise IO::TimeoutError if poll_result == 0 | ||
| Errno.handle_errno(Errno.errno) if poll_result == -1 |
There was a problem hiding this comment.
major: Looks like error handling isn't accurate. Wondering if it's easy to test these cases.
It seems Truffle::IOOperations.poll:
- returns
falsewhen it times out - calls
Errno.handle_errno(errno)and consequently raisesSystemCallErrorexception if there is error other thanEINTR - returns nonnegative
Integerotherwise
There was a problem hiding this comment.
As chatted about this - I'm attempting to update
from returningfalse to 0. The reason is - Posix's poll does return 0 on timeout (not exactly like this, when timeout happens on an EINTR, but the idea is pretty much equal). As well there is a lot of code doing poll(_) > 0 - which would raise with a boolean result.
And regarding:
calls Errno.handle_errno(errno) and consequently raises SystemCallError exception if there is error other than EINTR
Raising inside poll is fine, as far as I can tell we want that anyways. I would keep the check for -1 in read/write too, just to be safe and pragmatic.
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
607745e
to
5d9494f
Compare
andrykonchin
reviewed
Jun 8, 2023
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
5d9494f
to
a273dbf
Compare
andrykonchin
approved these changes
Jun 14, 2023
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
a273dbf
to
9b3fb03
Compare
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
2 times, most recently
from
af440ac
to
cda497d
Compare
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
2 times, most recently
from
de2a514
to
81dd5c6
Compare
eregon
approved these changes
Aug 11, 2023
Make timeout to be integer milliseconds in the loop
itarato
force-pushed
the
feature/PA-3039-IO-timeout
branch
from
81dd5c6
to
32f3793
Compare
eregon
approved these changes
Aug 16, 2023
andrykonchin
added
the
in-ci
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.
Source: #3039
Introduce IO#timeout= and IO#timeout which can cause
IO::TimeoutError to be raised if a blocking operation exceeds the
specified timeout. [Feature #18630]
Caveat
This PR does not contain updating the error type for
TCPSocket#openand#new. It seems we don't have the whole timeout logic implemented for that yet. I rather have them implemented in a separate PR to keep it clean.