[PATCH] Don't use deprecated api

[PATCH] Don't use deprecated api

[Hleb Valoshka]

This patch allows to use current Ruby API (rb_thread_call_without_gvl)
instead of deprecated `rb_thread_blocking_region'.

---
 ext/kgio/accept.c             | 7 +++++--
 ext/kgio/blocking_io_region.h | 3 +++
 ext/kgio/extconf.rb           | 8 ++++++--
 ext/kgio/tryopen.c            | 6 +++++-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/ext/kgio/accept.c b/ext/kgio/accept.c
index 67a8ded..ebfcf54 100644
--- a/ext/kgio/accept.c
+++ b/ext/kgio/accept.c
@@ -10,7 +10,9 @@ static VALUE cKgio_Socket;
 static VALUE mSocketMethods;
 static VALUE iv_kgio_addr;
 
-#if defined(__linux__) && defined(HAVE_RB_THREAD_BLOCKING_REGION)
+#if defined(__linux__) && \
+    (defined(HAVE_RB_THREAD_BLOCKING_REGION) || \
+     defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL))
 static int accept4_flags = SOCK_CLOEXEC;
 #else /* ! linux */
 static int accept4_flags = SOCK_CLOEXEC | SOCK_NONBLOCK;
@@ -76,7 +78,8 @@ static VALUE xaccept(void *ptr)
 	return (VALUE)rv;
 }
 
-#ifdef HAVE_RB_THREAD_BLOCKING_REGION
+#if defined HAVE_RB_THREAD_BLOCKING_REGION || \
+    defined HAVE_RB_THREAD_CALL_WITHOUT_GVL
 #  include <time.h>
 #  include "blocking_io_region.h"
 static int thread_accept(struct accept_args *a, int force_nonblock)
diff --git a/ext/kgio/blocking_io_region.h b/ext/kgio/blocking_io_region.h
index 30c7106..2a17c05 100644
--- a/ext/kgio/blocking_io_region.h
+++ b/ext/kgio/blocking_io_region.h
@@ -6,4 +6,7 @@ VALUE rb_thread_io_blocking_region(rb_blocking_function_t *, void *, int);
 #    define rb_thread_io_blocking_region(fn,data,fd) \
             rb_thread_blocking_region((fn),(data),RUBY_UBF_IO,0)
 #  endif
+#else
+#  define rb_thread_io_blocking_region(fn,data,fd) \
+          rb_thread_call_without_gvl((fn),(data),RUBY_UBF_IO,0)
 #endif
diff --git a/ext/kgio/extconf.rb b/ext/kgio/extconf.rb
index 5fb15ac..eaf1af2 100644
--- a/ext/kgio/extconf.rb
+++ b/ext/kgio/extconf.rb
@@ -46,8 +46,12 @@ have_func('rb_io_ascii8bit_binmode')
 have_func('rb_update_max_fd')
 have_func('rb_fd_fix_cloexec')
 have_func('rb_cloexec_open')
-have_func('rb_thread_blocking_region')
-have_func('rb_thread_io_blocking_region')
+if have_header('ruby/thread.h')
+  have_func('rb_thread_call_without_gvl')
+else
+  have_func('rb_thread_blocking_region')
+  have_func('rb_thread_io_blocking_region')
+end
 have_func('rb_str_set_len')
 have_func('rb_time_interval')
 have_func('rb_wait_for_single_fd')
diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c
index 902c745..bcf443d 100644
--- a/ext/kgio/tryopen.c
+++ b/ext/kgio/tryopen.c
@@ -38,7 +38,7 @@ static VALUE nogvl_open(void *ptr)
 	return (VALUE)rb_cloexec_open(o->pathname, o->flags, o->mode);
 }
 
-#ifndef HAVE_RB_THREAD_BLOCKING_REGION
+#if !defined HAVE_RB_THREAD_BLOCKING_REGION && !defined RUBY_UBF_IO
 #  define RUBY_UBF_IO ((void *)(-1))
 #  include "rubysig.h"
 typedef void rb_unblock_function_t(void *);
@@ -105,7 +105,11 @@ static VALUE s_tryopen(int argc, VALUE *argv, VALUE klass)
 	}
 
 retry:
+#ifdef HAVE_RB_THREAD_CALL_WITHOUT_GVL
+	fd = (int)rb_thread_call_without_gvl(nogvl_open, &o, RUBY_UBF_IO, 0);
+#else
 	fd = (int)rb_thread_blocking_region(nogvl_open, &o, RUBY_UBF_IO, 0);
+#endif
 	if (fd < 0) {
 		if (errno == EMFILE || errno == ENFILE || errno == ENOMEM) {
 			rb_gc();
-- 
1.8.5.2

Re: [PATCH] Don't use deprecated api

[Eric Wong]

Hleb Valoshka <375gnu@gmail.com> wrote:
> This patch allows to use current Ruby API (rb_thread_call_without_gvl)
> instead of deprecated `rb_thread_blocking_region'.

Thanks. I agree in principle, but the implementation needs to be
tweaked.  Comments inline.

>  ext/kgio/accept.c             | 7 +++++--
>  ext/kgio/blocking_io_region.h | 3 +++
>  ext/kgio/extconf.rb           | 8 ++++++--
>  ext/kgio/tryopen.c            | 6 +++++-
>  4 files changed, 19 insertions(+), 5 deletions(-)

connect.c needs to be updated for the MSG_FASTOPEN code.
I can do it if you can't test TFO on your system

>  static VALUE mSocketMethods;
>  static VALUE iv_kgio_addr;
>  
> -#if defined(__linux__) && defined(HAVE_RB_THREAD_BLOCKING_REGION)
> +#if defined(__linux__) && \
> +    (defined(HAVE_RB_THREAD_BLOCKING_REGION) || \
> +     defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL))
>  static int accept4_flags = SOCK_CLOEXEC;
>  #else /* ! linux */
>  static int accept4_flags = SOCK_CLOEXEC | SOCK_NONBLOCK;
> @@ -76,7 +78,8 @@ static VALUE xaccept(void *ptr)
>  	return (VALUE)rv;
>  }
>  
> -#ifdef HAVE_RB_THREAD_BLOCKING_REGION
> +#if defined HAVE_RB_THREAD_BLOCKING_REGION || \
> +    defined HAVE_RB_THREAD_CALL_WITHOUT_GVL

We should probably have a KGIO_ macro for both of these combined, so
it'll be harder to miss pieces like connect.c

> diff --git a/ext/kgio/extconf.rb b/ext/kgio/extconf.rb
> index 5fb15ac..eaf1af2 100644
> --- a/ext/kgio/extconf.rb
> +++ b/ext/kgio/extconf.rb
> @@ -46,8 +46,12 @@ have_func('rb_io_ascii8bit_binmode')
>  have_func('rb_update_max_fd')
>  have_func('rb_fd_fix_cloexec')
>  have_func('rb_cloexec_open')
> -have_func('rb_thread_blocking_region')
> -have_func('rb_thread_io_blocking_region')
> +if have_header('ruby/thread.h')
> +  have_func('rb_thread_call_without_gvl')
> +else
> +  have_func('rb_thread_blocking_region')
> +  have_func('rb_thread_io_blocking_region')
> +end

Even though rb_thread_io_blocking_region is not officially in the public
API, it is superior when an FD is known as it allows blocked threads
to be woken/interrupted on IO#close The socket stdlib uses it (as does
all the core IO methods).


We should check for all of these rb_thread_* functions unconditionally,
since sometimes these HAVE_* macros may be defined in headers regardless
of what we do in extconf.rb, so we shouldn't remove/change existing
checks for HAVE_* macros:

> -#ifndef HAVE_RB_THREAD_BLOCKING_REGION
> +#if !defined HAVE_RB_THREAD_BLOCKING_REGION && !defined RUBY_UBF_IO
>  #  define RUBY_UBF_IO ((void *)(-1))
>  #  include "rubysig.h"
>  typedef void rb_unblock_function_t(void *);


> @@ -105,7 +105,11 @@ static VALUE s_tryopen(int argc, VALUE *argv, VALUE klass)
>  	}
 
>  retry:
> +#ifdef HAVE_RB_THREAD_CALL_WITHOUT_GVL
> +	fd = (int)rb_thread_call_without_gvl(nogvl_open, &o, RUBY_UBF_IO, 0);
> +#else
>  	fd = (int)rb_thread_blocking_region(nogvl_open, &o, RUBY_UBF_IO, 0);
> +#endif

Please define a wrapper macro, I intensely dislike #ifdef inside
function bodies.  I cannot stand reading some MRI code because of this;
my preferred C style is from the Linux kernel
(Documentation/CodingStyle).

Re: [PATCH] Don't use deprecated api

[Eric Wong]

Btw, are you planning on providing a followup patch after
my comments?  I am interested in integrating this, thanks.

Re: [PATCH] Don't use deprecated api

[Hleb Valoshka]

On 1/17/14, Eric Wong <normalperson@yhbt.net> wrote:

> Btw, are you planning on providing a followup patch after
> my comments?  I am interested in integrating this, thanks.

Yes, but I currently busy and have no enough time to do that :(

Re: [PATCH] Don't use deprecated api

[Hleb Valoshka]

The patch is almost done (it was tested with mri1.8, 1.9.3 and 2.0),
but before I finish it, I should ask about correctness of the
following patch. Is this replacement is correct?


commit a6e30828439d5fb194ffe74b26a0ad06b96057c9
Author: Hleb Valoshka <375gnu@gmail.com>
Date:   Sun Jan 19 22:35:11 2014 +0300

    use rb_thread_io_blocking_region

    dunno whether this is correct replacement

diff --git a/ext/kgio/poll.c b/ext/kgio/poll.c
index 15774d8..5573247 100644
--- a/ext/kgio/poll.c
+++ b/ext/kgio/poll.c
@@ -5,6 +5,7 @@
 #include "broken_system_compat.h"
 #include <poll.h>
 #include <errno.h>
+#include "blocking_io_region.h"
 #ifdef HAVE_RUBY_ST_H
 #  include <ruby/st.h>
 #else
@@ -145,7 +146,7 @@ static VALUE do_poll(VALUE args)

 retry:
 	hash2pollfds(a);
-	nr = (int)rb_thread_blocking_region(nogvl_poll, a, RUBY_UBF_IO, NULL);
+	nr = (int)rb_thread_io_blocking_region(nogvl_poll, a, 0);
 	if (nr < 0) {
 		if (interrupted()) {
 			if (retryable(a)) {

Re: [PATCH] Don't use deprecated api

[Eric Wong]

Hleb Valoshka <375gnu@gmail.com> wrote:
> The patch is almost done (it was tested with mri1.8, 1.9.3 and 2.0),
> but before I finish it, I should ask about correctness of the
> following patch. Is this replacement is correct?

Not exactly, but it actually works...

The FD arg (0, stdin) means Ruby will wake up the polling thread if
that FD is closed, however:

1) poll may not be watching FD=0
2) things tend to break if you trie to close the standard FDs,
   so Ruby (MRI at least) doesn't actually close them

I might be inclined to accept this if there's a comment about the fd=0
being a dummy arg as well as the reasoning of using this function vs
rb_thread_call_without_gvl or rb_thread_blocking_region because the
former is detectable-but-unlinkable in 1.9.3, while the later is
deprecated in 2.x

>  retry:
>  	hash2pollfds(a);
> -	nr = (int)rb_thread_blocking_region(nogvl_poll, a, RUBY_UBF_IO, NULL);
> +	nr = (int)rb_thread_io_blocking_region(nogvl_poll, a, 0);
>  	if (nr < 0) {
>  		if (interrupted()) {
>  			if (retryable(a)) {

Re: [PATCH] Don't use deprecated api

[Hleb Valoshka]

On 1/20/14, Eric Wong <normalperson@yhbt.net> wrote:
> I might be inclined to accept this if there's a comment about the fd=0
> being a dummy arg as well as the reasoning of using this function vs
> rb_thread_call_without_gvl or rb_thread_blocking_region because the
> former is detectable-but-unlinkable in 1.9.3, while the later is
> deprecated in 2.x

So it seems to be better not to touch it.

The next message is updated patch, I've tested it with ruby 1.8.7p358,
1.9.3p484, 2.0.0p353, 2.1.0p0 and rubinius 2.0.0.n257.

With mri 2.x it uses rb_thread_call_without_gvl and
rb_thread_io_blocking_region, rb_thread_blocking_region and
rb_thread_io_blocking_region with 1.9.3, rb_thread_call_without_gvl
with rbx and old functions with 1.8.7.

Waiting for comments.

[PATCH] Don't use deprecated api

[Hleb Valoshka]

---
 ext/kgio/accept.c             |  8 ++++----
 ext/kgio/blocking_io_region.h |  9 ++++++---
 ext/kgio/connect.c            |  4 ++--
 ext/kgio/extconf.rb           |  2 ++
 ext/kgio/kgio.h               | 14 +++++++++++++-
 ext/kgio/tryopen.c            |  5 +++--
 6 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/ext/kgio/accept.c b/ext/kgio/accept.c
index 67a8ded..911e169 100644
--- a/ext/kgio/accept.c
+++ b/ext/kgio/accept.c
@@ -10,7 +10,7 @@ static VALUE cKgio_Socket;
 static VALUE mSocketMethods;
 static VALUE iv_kgio_addr;
 
-#if defined(__linux__) && defined(HAVE_RB_THREAD_BLOCKING_REGION)
+#if defined(__linux__) && defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL)
 static int accept4_flags = SOCK_CLOEXEC;
 #else /* ! linux */
 static int accept4_flags = SOCK_CLOEXEC | SOCK_NONBLOCK;
@@ -76,7 +76,7 @@ static VALUE xaccept(void *ptr)
 	return (VALUE)rv;
 }
 
-#ifdef HAVE_RB_THREAD_BLOCKING_REGION
+#ifdef KGIO_HAVE_THREAD_CALL_WITHOUT_GVL
 #  include <time.h>
 #  include "blocking_io_region.h"
 static int thread_accept(struct accept_args *a, int force_nonblock)
@@ -86,7 +86,7 @@ static int thread_accept(struct accept_args *a, int force_nonblock)
 	return (int)rb_thread_io_blocking_region(xaccept, a, a->fd);
 }
 
-#else /* ! HAVE_RB_THREAD_BLOCKING_REGION */
+#else /* ! KGIO_HAVE_THREAD_CALL_WITHOUT_GVL */
 #  include <rubysig.h>
 static int thread_accept(struct accept_args *a, int force_nonblock)
 {
@@ -103,7 +103,7 @@ static int thread_accept(struct accept_args *a, int force_nonblock)
 	TRAP_END;
 	return rv;
 }
-#endif /* ! HAVE_RB_THREAD_BLOCKING_REGION */
+#endif /* ! KGIO_HAVE_THREAD_CALL_WITHOUT_GVL */
 
 static void
 prepare_accept(struct accept_args *a, VALUE self, int argc, const VALUE *argv)
diff --git a/ext/kgio/blocking_io_region.h b/ext/kgio/blocking_io_region.h
index 30c7106..10e7533 100644
--- a/ext/kgio/blocking_io_region.h
+++ b/ext/kgio/blocking_io_region.h
@@ -1,8 +1,11 @@
-#ifdef HAVE_RB_THREAD_BLOCKING_REGION
-#  ifdef HAVE_RB_THREAD_IO_BLOCKING_REGION
+#ifdef KGIO_HAVE_THREAD_CALL_WITHOUT_GVL
+#  if   defined(HAVE_RB_THREAD_IO_BLOCKING_REGION)
 /* temporary API for Ruby 1.9.3 */
 VALUE rb_thread_io_blocking_region(rb_blocking_function_t *, void *, int);
-#  else
+#  elif defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL)
+#    define rb_thread_io_blocking_region(fn,data,fd) \
+            rb_thread_call_without_gvl((fn),(data),RUBY_UBF_IO,0)
+#  elif defined(HAVE_RB_THREAD_BLOCKING_REGION)
 #    define rb_thread_io_blocking_region(fn,data,fd) \
             rb_thread_blocking_region((fn),(data),RUBY_UBF_IO,0)
 #  endif
diff --git a/ext/kgio/connect.c b/ext/kgio/connect.c
index 2027795..ef1a396 100644
--- a/ext/kgio/connect.c
+++ b/ext/kgio/connect.c
@@ -143,7 +143,7 @@ static struct sockaddr *sockaddr_from(socklen_t *addrlen, VALUE addr)
 	return NULL;
 }
 
-#if defined(MSG_FASTOPEN) && defined(HAVE_RB_THREAD_BLOCKING_REGION)
+#if defined(MSG_FASTOPEN) && defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL)
 #ifndef HAVE_RB_STR_SUBSEQ
 #define rb_str_subseq rb_str_substr
 #endif
@@ -381,7 +381,7 @@ void init_kgio_connect(void)
 	rb_define_singleton_method(cKgio_Socket, "new", kgio_new, -1);
 	rb_define_singleton_method(cKgio_Socket, "connect", kgio_connect, 1);
 	rb_define_singleton_method(cKgio_Socket, "start", kgio_start, 1);
-#if defined(MSG_FASTOPEN) && defined(HAVE_RB_THREAD_BLOCKING_REGION)
+#if defined(MSG_FASTOPEN) && defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL)
 	rb_define_method(cKgio_Socket, "kgio_fastopen", fastopen, 2);
 #endif
 	/*
diff --git a/ext/kgio/extconf.rb b/ext/kgio/extconf.rb
index 5fb15ac..44888dd 100644
--- a/ext/kgio/extconf.rb
+++ b/ext/kgio/extconf.rb
@@ -46,6 +46,8 @@ have_func('rb_io_ascii8bit_binmode')
 have_func('rb_update_max_fd')
 have_func('rb_fd_fix_cloexec')
 have_func('rb_cloexec_open')
+have_header('ruby/thread.h')
+have_func('rb_thread_call_without_gvl', %w{ruby/thread.h})
 have_func('rb_thread_blocking_region')
 have_func('rb_thread_io_blocking_region')
 have_func('rb_str_set_len')
diff --git a/ext/kgio/kgio.h b/ext/kgio/kgio.h
index 983280d..0c689a3 100644
--- a/ext/kgio/kgio.h
+++ b/ext/kgio/kgio.h
@@ -7,6 +7,9 @@
 #else
 #  include <rubyio.h>
 #endif
+#ifdef HAVE_RUBY_THREAD_H
+#  include <ruby/thread.h>
+#endif
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/socket.h>
@@ -42,7 +45,16 @@ void kgio_autopush_send(VALUE);
 
 VALUE kgio_call_wait_writable(VALUE io);
 VALUE kgio_call_wait_readable(VALUE io);
-#if defined(HAVE_RB_THREAD_BLOCKING_REGION) && defined(HAVE_POLL)
+#if defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL)
+#  define KGIO_HAVE_THREAD_CALL_WITHOUT_GVL 1
+typedef  void *(*kgio_blocking_fn_t)(void*);
+#  define rb_thread_blocking_region(fn,data1,ubf,data2) \
+          rb_thread_call_without_gvl((kgio_blocking_fn_t)(fn),(data1),(ubf),(data2))
+#elif defined(HAVE_RB_THREAD_BLOCKING_REGION)
+#  define KGIO_HAVE_THREAD_CALL_WITHOUT_GVL 1
+#endif /* HAVE_RB_THREAD_CALL_WITHOUT_GVL || HAVE_RB_THREAD_BLOCKING_REGION */
+
+#if defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL) && defined(HAVE_POLL)
 #  define USE_KGIO_POLL
 #endif /* USE_KGIO_POLL */
 
diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c
index 902c745..1d8fffd 100644
--- a/ext/kgio/tryopen.c
+++ b/ext/kgio/tryopen.c
@@ -17,6 +17,7 @@
 #include <errno.h>
 #include "set_file_path.h"
 #include "ancient_ruby.h"
+#include "kgio.h"
 
 static ID id_for_fd, id_to_path, id_path;
 static st_table *errno2sym;
@@ -38,7 +39,7 @@ static VALUE nogvl_open(void *ptr)
 	return (VALUE)rb_cloexec_open(o->pathname, o->flags, o->mode);
 }
 
-#ifndef HAVE_RB_THREAD_BLOCKING_REGION
+#ifndef KGIO_HAVE_THREAD_CALL_WITHOUT_GVL
 #  define RUBY_UBF_IO ((void *)(-1))
 #  include "rubysig.h"
 typedef void rb_unblock_function_t(void *);
@@ -57,7 +58,7 @@ static VALUE my_thread_blocking_region(
 }
 #define rb_thread_blocking_region(fn,data1,ubf,data2) \
         my_thread_blocking_region((fn),(data1),(ubf),(data2))
-#endif /* ! HAVE_RB_THREAD_BLOCKING_REGION */
+#endif /* ! KGIO_HAVE_THREAD_CALL_WITHOUT_GVL */
 
 /*
  * call-seq:
-- 
1.8.5.3

Re: [PATCH] Don't use deprecated api

[Eric Wong]

Thanks!  Signed-off and pushed
(commit 008483785d1a5bb801219af8afbb77be18b9bef1)