[ruby-core:66385] [ruby-trunk - Feature #10532] [Open] [PATCH] accept_nonblock supports "exception: false"

[ruby-core:66385] [ruby-trunk - Feature #10532] [Open] [PATCH] accept_nonblock supports "exception: false"

[normalperson]

Issue #10532 has been reported by Eric Wong.

----------------------------------------
Feature #10532: [PATCH] accept_nonblock supports "exception: false"
https://bugs.ruby-lang.org/issues/10532

* Author: Eric Wong
* Status: Open
* Priority: Normal
* Assignee: Yukihiro Matsumoto
* Category: ext
* Target version: current: 2.2.0
----------------------------------------
This is analogous to functionality found in IO#read_nonblock and
IO#wait_nonblock.  Raising exceptions for common failures on
non-blocking servers is expensive and makes $DEBUG too noisy.

This also increases performance slightly, see patch for benchmarks.

I also intend to support this in the "openssl" extension as well as
supporting connect_nonblock if this is accepted.


---Files--------------------------------
0001-accept_nonblock-supports-exception-false.patch (9.72 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:66386] Re: [ruby-trunk - Feature #10532] [Open] [PATCH] accept_nonblock supports "exception: false"

[Eric Wong]

normalperson@yhbt.net wrote:
> I also intend to support this in the "openssl" extension as well as

accept_nonblock for openssl:
http://80x24.org/spew/m/f453956f3b04d2445e54d3141cc564a2529ce0f7.txt

Also pushed to the "more_nonblock" branch on git://80x24.org/ruby

[ruby-core:66387] [ruby-trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[normalperson]

Issue #10532 has been updated by Eric Wong.


 normalperson@yhbt.net wrote:
 > I also intend to support this in the "openssl" extension as well as
 
 accept_nonblock for openssl:
 http://80x24.org/spew/m/f453956f3b04d2445e54d3141cc564a2529ce0f7.txt
 
 Also pushed to the "more_nonblock" branch on git://80x24.org/ruby

----------------------------------------
Feature #10532: [PATCH] accept_nonblock supports "exception: false"
https://bugs.ruby-lang.org/issues/10532#change-50030

* Author: Eric Wong
* Status: Open
* Priority: Normal
* Assignee: Yukihiro Matsumoto
* Category: ext
* Target version: current: 2.2.0
----------------------------------------
This is analogous to functionality found in IO#read_nonblock and
IO#wait_nonblock.  Raising exceptions for common failures on
non-blocking servers is expensive and makes $DEBUG too noisy.

This also increases performance slightly, see patch for benchmarks.

I also intend to support this in the "openssl" extension as well as
supporting connect_nonblock if this is accepted.


---Files--------------------------------
0001-accept_nonblock-supports-exception-false.patch (9.72 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:68485] [Ruby trunk - Feature #10532] [Feedback] [PATCH] accept_nonblock supports "exception: false"

[matz]

Issue #10532 has been updated by Yukihiro Matsumoto.

Status changed from Open to Feedback

I like the basic idea. Once someone review the patch (Nobu, are you willing?), it's OK to merge.

Matz.


----------------------------------------
Feature #10532: [PATCH] accept_nonblock supports "exception: false"
https://bugs.ruby-lang.org/issues/10532#change-51822

* Author: Eric Wong
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
This is analogous to functionality found in IO#read_nonblock and
IO#wait_nonblock.  Raising exceptions for common failures on
non-blocking servers is expensive and makes $DEBUG too noisy.

This also increases performance slightly, see patch for benchmarks.

I also intend to support this in the "openssl" extension as well as
supporting connect_nonblock if this is accepted.


---Files--------------------------------
0001-accept_nonblock-supports-exception-false.patch (9.72 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:68491] [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[nobu]

Issue #10532 has been updated by Nobuyoshi Nakada.


Why only OpenSSL?
Doesn't `Socket#accept_nonblock` need it?

----------------------------------------
Feature #10532: [PATCH] accept_nonblock supports "exception: false"
https://bugs.ruby-lang.org/issues/10532#change-51828

* Author: Eric Wong
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
This is analogous to functionality found in IO#read_nonblock and
IO#wait_nonblock.  Raising exceptions for common failures on
non-blocking servers is expensive and makes $DEBUG too noisy.

This also increases performance slightly, see patch for benchmarks.

I also intend to support this in the "openssl" extension as well as
supporting connect_nonblock if this is accepted.


---Files--------------------------------
0001-accept_nonblock-supports-exception-false.patch (9.72 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:68492] Re: [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[Eric Wong]

nobu: there's two patches, the one for plain *Socket is attached to
the ticket; OpenSSL one was added later.

[ruby-core:68493] [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[nobu]

Issue #10532 has been updated by Nobuyoshi Nakada.


Sorry, I've read the second link only.
Both look fine.

----------------------------------------
Feature #10532: [PATCH] accept_nonblock supports "exception: false"
https://bugs.ruby-lang.org/issues/10532#change-51831

* Author: Eric Wong
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
This is analogous to functionality found in IO#read_nonblock and
IO#wait_nonblock.  Raising exceptions for common failures on
non-blocking servers is expensive and makes $DEBUG too noisy.

This also increases performance slightly, see patch for benchmarks.

I also intend to support this in the "openssl" extension as well as
supporting connect_nonblock if this is accepted.


---Files--------------------------------
0001-accept_nonblock-supports-exception-false.patch (9.72 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:68503] Re: [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[Eric Wong]

Committed as r49948.

I tried to cleanup to use rb_get_kwargs, but got stuck on a possible
bug in rb_get_kwargs.

I think the st_delete call in rb_get_kwargs interacts badly with with
keyword optimizations ko1 did in 2.2.  My work-in-progress patch
gets stuck on the OpenSSL read_nonblock (from
ext/openssl/lib/openssl/buffering.rb) in test/openssl/test_ssl.rb

Work-in-progress is below, see the FIXME area in my patch which starts
with "if (1)" (tests are OK with "if (0)")

http://80x24.org/spew/m/20150312030016.GA10183%40dcvr.yhbt.net.txt

[ruby-core:68504] Re: [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[Nobuyoshi Nakada]

On 2015/03/12 12:08, Eric Wong wrote:
> I think the st_delete call in rb_get_kwargs interacts badly with with
> keyword optimizations ko1 did in 2.2.  My work-in-progress patch
> gets stuck on the OpenSSL read_nonblock (from
> ext/openssl/lib/openssl/buffering.rb) in test/openssl/test_ssl.rb

It is an intentional behavior.

Add this after `rb_get_kwargs()` call.

    if (except != Qundef)
	rb_hash_aset(opts, ID2SYM(id_exception), except);

-- 
Nobu Nakada

[ruby-core:68507] Re: [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[Nobuyoshi Nakada]

On 2015/03/12 17:38, Nobuyoshi Nakada wrote:
> On 2015/03/12 12:08, Eric Wong wrote:
>> I think the st_delete call in rb_get_kwargs interacts badly with with
>> keyword optimizations ko1 did in 2.2.  My work-in-progress patch
>> gets stuck on the OpenSSL read_nonblock (from
>> ext/openssl/lib/openssl/buffering.rb) in test/openssl/test_ssl.rb
> 
> It is an intentional behavior.
> 
> Add this after `rb_get_kwargs()` call.
> 
>     if (except != Qundef)
> 	rb_hash_aset(opts, ID2SYM(id_exception), except);

Or you can just `rb_hash_lookup2(opts, ID2SYM(id_exception), Qundef)`,
if you want to pass the whole options without checking.

-- 
Nobu Nakada

[ruby-core:68511] Re: [Ruby trunk - Feature #10532] [PATCH] accept_nonblock supports "exception: false"

[Eric Wong]

Nobuyoshi Nakada <nobu@ruby-lang.org> wrote:
> On 2015/03/12 17:38, Nobuyoshi Nakada wrote:
> > On 2015/03/12 12:08, Eric Wong wrote:
> >> I think the st_delete call in rb_get_kwargs interacts badly with with
> >> keyword optimizations ko1 did in 2.2.  My work-in-progress patch
> >> gets stuck on the OpenSSL read_nonblock (from
> >> ext/openssl/lib/openssl/buffering.rb) in test/openssl/test_ssl.rb
> > 
> > It is an intentional behavior.
> > 
> > Add this after `rb_get_kwargs()` call.
> > 
> >     if (except != Qundef)
> > 	rb_hash_aset(opts, ID2SYM(id_exception), except);

That seems tricky.  We should to document it, at least:

diff --git a/README.EXT b/README.EXT
index f24029c..f87ad85 100644
--- a/README.EXT
+++ b/README.EXT
@@ -1308,7 +1308,8 @@ rb_scan_args(int argc, VALUE *argv, const char *fmt, ...) ::
 int rb_get_kwargs(VALUE keyword_hash, const ID *table, int required, int optional, VALUE *values)
 
   Retrieves argument VALUEs bound to keywords, which directed by +table+
-  into +values+.  First +required+ number of IDs referred by +table+ are
+  into +values+, deleting retrieved entries from +keyword_hash+ along
+  the way.  First +required+ number of IDs referred by +table+ are
   mandatory, and succeeding +optional+ (- +optional+ - 1 if
   +optional+ is negative) number of IDs are optional.  If a
   mandatory key is not contained in +keyword_hash+, raises "missing
@@ -1316,7 +1317,8 @@ int rb_get_kwargs(VALUE keyword_hash, const ID *table, int required, int optiona
   +keyword_hash+, the corresponding element in +values+ is not changed.
   If +optional+ is negative, rest of +keyword_hash+ are stored in the
   next to optional +values+ as a new Hash, otherwise raises "unknown
-  keyword" +ArgumentError+.
+  keyword" +ArgumentError+.  If +values+ is NULL, +keyword_hash+ is
+  not modified and only error checking is performed.
 
 VALUE rb_extract_keywords(VALUE *original_hash)
 

I'll commit the above unless somebody else has better ideas.

> Or you can just `rb_hash_lookup2(opts, ID2SYM(id_exception), Qundef)`,
> if you want to pass the whole options without checking.

Alright, I'll do that (not much different from current rb_hash_aref).