Re: [PATCH] Don't use deprecated api

[Eric Wong <normalperson@yhbt.net>]

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).