https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112012-03-16T13:05:25ZRuby Issue Tracking SystemRuby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=246312012-03-16T13:05:25Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>I should have mentioned that JRuby has been running this way for a while (on master) and we have had no reports of incompatibility. The concrete subclass is_a EAGAIN and is_a WaitReadable, so all typical exception-handling patterns work correctly. The only cases that break are cases that check e.class == EAGAIN, which is probably not a good pattern anyway.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=246642012-03-17T09:29:11Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>Charles Nutter <a href="mailto:headius@headius.com" class="email">headius@headius.com</a> wrote:</p>
<blockquote>
<p>I should have mentioned that JRuby has been running this way for a<br>
while (on master) and we have had no reports of incompatibility. The<br>
concrete subclass is_a EAGAIN and is_a WaitReadable, so all typical<br>
exception-handling patterns work correctly.</p>
</blockquote>
<p>Good to know. I like this change[1]</p>
<blockquote>
<p>The only cases that break are cases that check e.class == EAGAIN,<br>
which is probably not a good pattern anyway.</p>
</blockquote>
<p>I also noticed some test failures with your patch. (But I think<br>
the minor incompatibility is acceptable for 2.0.0)</p>
<h2>test_read_nonblock(OpenSSL::TestPair) [$top_srcdir/test/openssl/test_pair.rb:145]:<br>
[OpenSSL::SSL::SSLError] exception expected, not<br>
Class: <a href="OpenSSL::SSL::SSLErrorReaable" class="external">OpenSSL::SSL::SSLErrorReaable</a><br>
Message: <"read would block"><br>
---Backtrace---<br>
$top_srcdir/trunk/.ext/common/openssl/buffering.rb:174:in <code>sysread_nonblock' $top_srcdir/trunk/.ext/common/openssl/buffering.rb:174:in </code>read_nonblock'<br>
$top_srcdir/test/openssl/test_pair.rb:147:in `block (2 levels) in test_read_nonblock'</h2>
<h2>test_dgram_pair(TestSocket_UNIXSocket) [$top_srcdir/test/socket/test_unix.rb:348]:<br>
[Errno::EAGAIN] exception expected, not<br>
Class: <a href="IO::EAGAINReadable" class="external">IO::EAGAINReadable</a><br>
Message: <"Resource temporarily unavailable - recvfrom(2) would block"><br>
---Backtrace---<br>
$top_srcdir/test/socket/test_unix.rb:348:in <code>recv_nonblock' $top_srcdir/test/socket/test_unix.rb:348:in </code>block in test_dgram_pair'</h2>
<p>[1] I'd like an exception-avoiding API more, though :)</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=246732012-03-17T17:59:13Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>On Fri, Mar 16, 2012 at 7:24 PM, Eric Wong <a href="mailto:normalperson@yhbt.net" class="email">normalperson@yhbt.net</a> wrote:</p>
<blockquote>
<blockquote>
<p>The only cases that break are cases that check e.class == EAGAIN,<br>
which is probably not a good pattern anyway.</p>
</blockquote>
<p>I also noticed some test failures with your patch. (But I think<br>
the minor incompatibility is acceptable for 2.0.0)</p>
<p>test_read_nonblock(OpenSSL::TestPair) [$top_srcdir/test/openssl/test_pair.rb:145]:<br>
[OpenSSL::SSL::SSLError] exception expected, not<br>
Class: <a href="OpenSSL::SSL::SSLErrorReaable" class="external">OpenSSL::SSL::SSLErrorReaable</a><br>
...<br>
test_dgram_pair(TestSocket_UNIXSocket) [$top_srcdir/test/socket/test_unix.rb:348]:<br>
[Errno::EAGAIN] exception expected, not<br>
Class: <a href="IO::EAGAINReadable" class="external">IO::EAGAINReadable</a></p>
</blockquote>
<p>Yes, these are expected failures in my eyes, and not indicative of<br>
typical exception-handling or nonblocking-IO use cases.</p>
<p>And I agree 100% about adding an exception-free nonblocking API. This<br>
plasters over the problems 1.9.x introduced, but doesn't solve the<br>
root cause.</p>
<ul>
<li>Charlie</li>
</ul> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=246792012-03-18T07:53:12Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>I thought about that...it would still be better to add EWOULDBLOCKReadable,<br>
etc, than to pay the .extend toll every time.<br>
On Mar 17, 2012 10:28 AM, "Tanaka Akira" <a href="mailto:akr@fsij.org" class="email">akr@fsij.org</a> wrote:</p>
<blockquote>
<p>2012/3/16 Charles Nutter <a href="mailto:headius@headius.com" class="email">headius@headius.com</a>:</p>
<blockquote>
<p>The nonblocking IO operations started extending WaitReadable or<br>
WaitWritable into the Errno::EAGAIN instance some time during the 1.9<br>
series. This has a rather high cost, since a singleton class must be<br>
created and the global method cache must be flushed.</p>
<p>The attached patch instead creates two new classes of the following<br>
form, and raises them rather than raising a singleton EAGAIN:</p>
</blockquote>
<p>EWOULDBLOCK is a different from EAGAIN on some platforms.<br>
(HP-UX, for example.)</p>
<h2>I think your patch breaks applications which rescue only EWOULDBLOCK,<br>
on such platforms.</h2>
<p>Tanaka Akira</p>
</blockquote> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=249222012-03-19T00:23:17Znahi (Hiroshi Nakamura)nakahiro@gmail.com
<ul></ul><p>Charles, do you create a patch to introduce 4 constants (+2 for<br>
EWOULDBLOCK) ? We discussed this issue today and we concluded that<br>
that's better than anonymous class caching (to avoid Marshalling<br>
issue.)</p>
<p>I think I can take care of openssl change.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=250492012-03-23T02:37:57Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>Ok, will do.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=250512012-03-23T03:38:40Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>File</strong> <a href="/attachments/2523">eagain_readwrite.diff</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/2523/eagain_readwrite.diff">eagain_readwrite.diff</a> added</li></ul><p>Updated patch with the following changes:</p>
<ul>
<li>rb_eEWOULDBLOCKReadable and rb_eEWOULDBLOCKWritable added</li>
<li>EWOULDBLOCK versions are set to EAGAIN versions if EAGAIN == EWOULDBLOCK</li>
<li>renamed function rb_readwrite_sys_fail to match error.c names better</li>
<li>reverted OSSL changes so nahi can do them right</li>
<li>fixed the one test (test/socket/test_unix.rb) that expected an exact exception</li>
</ul>
<p>Notes:</p>
<ul>
<li>assert_raise should probably use === instead of == to better match rescue logic</li>
<li>more tests for this logic are needed(!!!)</li>
<li>I have not tested RubySpec yet</li>
<li>I can't test on HP/UX or other systems where EAGAIN != EWOULDBLOCK</li>
</ul> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=257712012-04-10T13:57:19Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul><li><strong>Tracker</strong> changed from <i>Bug</i> to <i>Feature</i></li><li><strong>Status</strong> changed from <i>Open</i> to <i>Assigned</i></li><li><strong>Assignee</strong> set to <i>matz (Yukihiro Matsumoto)</i></li></ul><p>I think it is suitable to handle this issue in the feature tracker.</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=329742012-11-17T01:27:43Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>Seven months and no activity. This is not a breaking change and it could improve performance of nonblocking IO operations a lot. Any reason not to incorporate it?</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=330502012-11-18T13:44:33Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>I have made the additional OpenSSL changes in the following JRuby commit: <a href="https://github.com/jruby/jruby/commit/8b022c896ea0d76f876e458229f4f150893295b9" class="external">https://github.com/jruby/jruby/commit/8b022c896ea0d76f876e458229f4f150893295b9</a></p>
<p>The new exceptions are SSLErrorReadable and SSLErrorWritable. I am not married to these names, but unfortunately I do not wish to continue perpetuating the performance impact of extending these exceptions every time.</p>
<p>The current behavior of MRI to extend WaitReadable and WaitWritable every time is a <em>bug</em>. It needs to be fixed.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=337852012-11-24T13:51:49Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul><li><strong>Target version</strong> set to <i>2.6</i></li></ul><p>I postpone this ticket to next minor. Very sorry.<br>
If Eric Wong were a committer, I would leave this to him.</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=346422012-12-12T09:23:16Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>"mame (Yusuke Endoh)" <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a> wrote:</p>
<blockquote>
<p>I postpone this ticket to next minor. Very sorry.<br>
If Eric Wong were a committer, I would leave this to him.</p>
</blockquote>
<p>Looks like Charles Nutter will be a committer soon and can handle this.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=376332013-03-16T02:24:45Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>mame or matz: Is this change approved? If so, I can proceed with a patch that includes what I've already posted plus OpenSSL changes.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=376562013-03-16T10:05:40Zdrbrain (Eric Hodel)drbrain@segment7.net
<ul></ul><p>Since a committer nominee was given permission to commit I think you should commit it</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381152013-04-03T02:23:24Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>Working on this today. I will post an updated patch for review before committing.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381162013-04-03T02:24:17Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>Category</strong> set to <i>core</i></li><li><strong>Assignee</strong> changed from <i>matz (Yukihiro Matsumoto)</i> to <i>headius (Charles Nutter)</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>70</i></li></ul> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381212013-04-03T07:20:24Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>File</strong> <a href="/attachments/3636">eagain_readwrite.diff</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/3636/eagain_readwrite.diff">eagain_readwrite.diff</a> added</li></ul><p>Ok, here's the updated patch. Here's a list of changes from the last time I updated:</p>
<ul>
<li>
<p>Names have been changed to EAGAINWaitReadable, etc, from EAGAINReadable. It seemed clearer to indicate the full name of the module, especially in the SSL case where it would have been SSLErrorReadable. Weird.</p>
</li>
<li>
<p>OpenSSL has been patched to have SSLErrorWaitReadable and SSLErrorWaitWritable. Note that I am not married to any of these names, if there's better suggestions.</p>
</li>
<li>
<p>Socket has been patched to use rb_readwrite_sys_fail for EINPROGRESS + WaitWritable. I added EINPROGRESSWaitWritable and EINPROGRESSWaitReadable because it uses rb_readwrite_sys_fail and I didn't think that method should only handle the writable version</p>
</li>
<li>
<p>rb_readwrite_sys_fail has been expanded to include EINPROGRESS versions of WaitReadable and WaitWritable. Note this is added as a public API, to give users a path away from rb_mod_sys_fail*.</p>
</li>
<li>
<p>Logic that switches based on ERRNO modified to use switch/case</p>
</li>
<li>
<p>Tests that checked for exact error types of EAGAIN have been modified to check for the WaitReadable/Writable subclasses.</p>
</li>
</ul>
<p>make test-all passes. Please review and comment.</p>
<p>Note that I'd also like to lobby for this change to be applied to 1.9.3, perhaps without the public API. Lots of people are going to be on 1.9.3 for a long time before moving to 2.0.x, and they'd like to have this improvement.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381242013-04-03T07:37:06ZAnonymous
<ul></ul><p>=begin<br>
Crazy idea, but could you avoid introducing a new class by caching the iclass?</p>
<p>So instead of something like:</p>
<pre><code>/* when exception is raised */
VALUE exc = ossl_exc_new(eSSLError, "read would block");
rb_extend_object(exc, rb_mWaitWritable);
rb_exc_raise(exc);
</code></pre>
<p>You might do something like this:</p>
<pre><code>VALUE cached_ssl_error_wait_writable_iclass;
/* once off setup during vm boot */
VALUE exc = ossl_exc_new(eSSLError, "read would block");
rb_extend_object(exc, rb_mWaitWritable);
cached_ssl_error_wait_writable_iclass = RBASIC(exc)->klass;
/* when exception is raised */
VALUE exc = ossl_exc_new(eSSLError, "read would block");
RBASIC(exc)->klass = cached_ssl_error_wait_writable_iclass;
rb_exc_raise(exc);
</code></pre>
<p>The benefit of this is that you don't have to change the behaviour that's visible from Ruby land.<br>
=end</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381252013-04-03T07:46:07Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>charliesome: Yeah, I was thinking about that as I created this patch.</p>
<p>It's possible, but it introduces a rather strange oddity: you'll have multiple exceptions floating around that look like singletons but are actually the same singleton class. If anyone adds methods to them it will add methods to all of them, but there's no way to know you're doing it. I believe this would mean you can't create proper isolated singleton instances/classes of these exception objects.</p>
<p>Are there other places in MRI where this pattern is used? If it's not being done elsewhere, I'd be reluctant to do it here.</p>
<p>The only behavior you have to change from Ruby land would be not testing for EAGAIN == exception. EAGAIN#===, WaitReadable#===, kind_of? and so on work just as they did before. I had to change the tests only because assert_raise does an == check internally (and it arguably should do === or kind_of? since that's closer to what rescue does).</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381262013-04-03T07:47:03Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>And by EAGAIN == exception I meant EAGAIN == exception.class, of course.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381272013-04-03T08:53:11Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>"headius (Charles Nutter)" <a href="mailto:headius@headius.com" class="email">headius@headius.com</a> wrote:</p>
<blockquote>
<p>make test-all passes. Please review and comment.</p>
</blockquote>
<blockquote>
<p>--- io.c (revision 40073)<br>
+++ io.c (working copy)<br>
@@ -133,6 +133,15 @@<br>
</p>
<p>+VALUE rb_eEAGAINWaitReadable;<br>
+VALUE rb_eEAGAINWaitWritable;<br>
+VALUE rb_eEWOULDBLOCKWaitReadable;<br>
+VALUE rb_eEWOULDBLOCKWaitWritable;<br>
+VALUE rb_eEINPROGRESSWaitWritable;<br>
+VALUE rb_eEINPROGRESSWaitReadable;</p>
</blockquote>
<p>New globals introduced don't appear to be used outside of io.c,<br>
so they can be made static (unless you intend to reference them<br>
outside of io.c in the future)</p>
<blockquote>
<p>--- ext/openssl/ossl_ssl.c (revision 40073)<br>
+++ ext/openssl/ossl_ssl.c (working copy)<br>
@@ -26,6 +26,8 @@</p>
<p>VALUE mSSL;<br>
VALUE eSSLError;<br>
+VALUE eSSLErrorWaitReadable;<br>
+VALUE eSSLErrorWaitWritable;<br>
VALUE cSSLContext;<br>
VALUE cSSLSocket;</p>
</blockquote>
<p>Likewise for these globals.</p>
<p>Otherwise, your patch looks good. Thanks!</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381332013-04-03T10:23:07Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>normalperson (Eric Wong) wrote:</p>
<blockquote>
<blockquote>
<p>+VALUE rb_eEAGAINWaitReadable;<br>
+VALUE rb_eEAGAINWaitWritable;<br>
+VALUE rb_eEWOULDBLOCKWaitReadable;<br>
+VALUE rb_eEWOULDBLOCKWaitWritable;<br>
+VALUE rb_eEINPROGRESSWaitWritable;<br>
+VALUE rb_eEINPROGRESSWaitReadable;</p>
</blockquote>
<p>New globals introduced don't appear to be used outside of io.c,<br>
so they can be made static (unless you intend to reference them<br>
outside of io.c in the future)</p>
</blockquote>
<p>Good call, will fix.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=381362013-04-03T10:55:38Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>File</strong> <a href="/attachments/3638">eagain_readwrite.diff</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/3638/eagain_readwrite.diff">eagain_readwrite.diff</a> added</li></ul><p>Updated patch with new exception variables made static.</p> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=382382013-04-05T08:58:25Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>I am prepared to commit this fix if there's no other commentary.</p>
<p>A quick benchmark to show it in action: <a href="https://gist.github.com/headius/5315475" class="external">https://gist.github.com/headius/5315475</a></p>
<pre><code>$ ./ruby -I lib:ext:. nonblock_bench.rb
0.940000 0.170000 1.110000 ( 1.113261)
0.940000 0.170000 1.110000 ( 1.104994)
0.940000 0.160000 1.100000 ( 1.108880)
0.950000 0.170000 1.120000 ( 1.107059)
0.970000 0.170000 1.140000 ( 1.148363)
$ ./ruby -I lib:ext:.:ext/socket nonblock_bench.rb
0.660000 0.140000 0.800000 ( 0.808189)
0.680000 0.150000 0.830000 ( 0.814997)
0.670000 0.140000 0.810000 ( 0.811901)
0.670000 0.140000 0.810000 ( 0.804447)
0.670000 0.140000 0.810000 ( 0.817231)
</code></pre> Ruby master - Feature #6154: Eliminate extending WaitReadable/Writable at runtimehttps://bugs.ruby-lang.org/issues/6154?journal_id=383712013-04-09T04:58:58Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>Status</strong> changed from <i>Assigned</i> to <i>Closed</i></li><li><strong>% Done</strong> changed from <i>70</i> to <i>100</i></li></ul><p>This issue was solved with changeset r40195.<br>
Charles, thank you for reporting this issue.<br>
Your contribution to Ruby is greatly appreciated.<br>
May Ruby be with you.</p>
<hr>
<p>Fix <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Eliminate extending WaitReadable/Writable at runtime (Closed)" href="https://bugs.ruby-lang.org/issues/6154">#6154</a> by introducing new EAGAIN/EWOULDBLOCK/EINPROGRESS<br>
subclasses that include WaitReadable or WaitWritable rather than<br>
extending them into the exception object each time.</p>
<ul>
<li>error.c: Capture EGAIN, EWOULDBLOCK, EINPROGRESS exceptions and<br>
export them for use in WaitReadable/Writable exceptions.</li>
<li>io.c: Create versions of EAGAIN, EWOULDBLOCK, EINPROGRESS that<br>
include WaitReadable and WaitWritable. Add rb_readwrite_sys_fail<br>
for nonblocking failures using those exceptions. Use that<br>
function in io_getpartial and io_write_nonblock instead of<br>
rb_mod_sys_fail</li>
<li>ext/openssl/ossl_ssl.c: Add new SSLError subclasses that include<br>
WaitReadable and WaitWritable. Use those classes for<br>
write_would_block and read_would_block instead of rb_mod_sys_fail.</li>
<li>ext/socket/ancdata.c: Use rb_readwrite_sys_fail instead of<br>
rb_mod_sys_fail in bsock_sendmsg_internal and<br>
bsock_recvmsg_internal.</li>
<li>ext/socket/init.c: Use rb_readwrite_sys_fail instead of<br>
rb_mod_sys_fail in rsock_s_recvfrom_nonblock and<br>
rsock_s_connect_nonblock.</li>
<li>ext/socket/socket.c: Use rb_readwrite_sys_fail instead of<br>
rb_mod_sys_fail in sock_connect_nonblock.</li>
<li>include/ruby/ruby.h: Export rb_readwrite_sys_fail for use instead<br>
of rb_mod_sys_fail. Introduce new constants RB_IO_WAIT_READABLE and<br>
RB_IO_WAIT_WRITABLE for first arg to rb_readwrite_sys_fail.</li>
</ul>