Improve IO#close performance and semantics.

doc/hacking/conventions.md

@@ -0,0 +1,32 @@
+# Naming Conventions
+
+This document gives an overview of naming conventions.
+
+## Avoid Exposing `struct`
+
+We should avoid exposing the internals of `struct` as a public interface unless absolutely necessary.
+
+## Avoid Naming With `_t` Suffix
+
+According to the [POSIX specification](https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html), top level names ending in `_t` are reserved.
+
+## Avoid Redundant Names
+
+Avoid naming structs like this:
+
+``` c
+struct my_struct {} // redundant use of struct
+struct my_struct_t {} // reserved use of _t
+```
+
+Both of these are missing `rb_` prefix which should be standard part of public interface.
+
+## Public / Internal Interfaces
+
+Generally speaking, for a given Ruby object, e.g. `IO::Buffer` you would expect to use names like this:
+
+```c
+// ruby/include/io/buffer.h
+
+VALUE rb_io_buffer_new(void *base, size_t size, enum rb_io_buffer_flags flags);
+```

ext/socket/basicsocket.c

@@ -597,7 +597,7 @@ rsock_bsock_send(int argc, VALUE *argv, VALUE socket)
         rb_io_wait(socket, RB_INT2NUM(RUBY_IO_WRITABLE), Qnil);
 #endif
 
-        ssize_t n = (ssize_t)BLOCKING_REGION_FD(func, &arg);
+        ssize_t n = (ssize_t)rb_thread_io_blocking_region(fptr, func, &arg);
 
         if (n >= 0) return SSIZET2NUM(n);
 

ext/socket/init.c

@@ -101,6 +101,7 @@ rsock_send_blocking(void *data)
 }
 
 struct recvfrom_arg {
+    rb_io_t *fptr;
     int fd, flags;
     VALUE str;
     size_t length;
@@ -144,7 +145,7 @@ recvfrom_locktmp(VALUE v)
 {
     struct recvfrom_arg *arg = (struct recvfrom_arg *)v;
 
-    return rb_thread_io_blocking_region(recvfrom_blocking, arg, arg->fd);
+    return rb_thread_io_blocking_region(arg->fptr, recvfrom_blocking, arg);
 }
 
 VALUE
@@ -173,6 +174,7 @@ rsock_s_recvfrom(VALUE socket, int argc, VALUE *argv, enum sock_recv_type from)
         rb_raise(rb_eIOError, "recv for buffered IO");
     }
 
+    arg.fptr = fptr;
     arg.fd = fptr->fd;
     arg.alen = (socklen_t)sizeof(arg.buf);
     arg.str = str;
@@ -549,19 +551,23 @@ socks_connect_blocking(void *data)
 #endif
 
 int
-rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks, struct timeval *timeout)
+rsock_connect(VALUE io, const struct sockaddr *sockaddr, int len, int socks, struct timeval *timeout)
 {
     int status;
     rb_blocking_function_t *func = connect_blocking;
+    int descriptor = rb_io_descriptor(io);
     struct connect_arg arg;
 
-    arg.fd = fd;
+    rb_io_t *fptr;
+    GetOpenFile(io, fptr);
+
+    arg.fd = descriptor;
     arg.sockaddr = sockaddr;
     arg.len = len;
 #if defined(SOCKS) && !defined(SOCKS5)
     if (socks) func = socks_connect_blocking;
 #endif
-    status = (int)BLOCKING_REGION_FD(func, &arg);
+    status = (int)rb_thread_io_blocking_region(fptr, func, &arg);
 
     if (status < 0) {
         switch (errno) {
@@ -573,7 +579,7 @@ rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks, struc
 #ifdef EINPROGRESS
           case EINPROGRESS:
 #endif
-            return wait_connectable(fd, timeout);
+            return wait_connectable(descriptor, timeout);
         }
     }
     return status;
@@ -692,7 +698,7 @@ rsock_s_accept(VALUE klass, VALUE io, struct sockaddr *sockaddr, socklen_t *len)
 #ifdef RSOCK_WAIT_BEFORE_BLOCKING
     rb_io_wait(fptr->self, RB_INT2NUM(RUBY_IO_READABLE), Qnil);
 #endif
-    peer = (int)BLOCKING_REGION_FD(accept_blocking, &accept_arg);
+    peer = (int)rb_thread_io_blocking_region(fptr, accept_blocking, &accept_arg);
     if (peer < 0) {
         int error = errno;
 

ext/socket/ipsocket.c

@@ -60,21 +60,20 @@ init_inetsock_internal(VALUE v)
         tv = &tv_storage;
     }
 
-    arg->remote.res = rsock_addrinfo(arg->remote.host, arg->remote.serv,
-                                     family, SOCK_STREAM,
-                                     (type == INET_SERVER) ? AI_PASSIVE : 0);
-
+    arg->remote.res = rsock_addrinfo(arg->remote.host, arg->remote.serv, family, SOCK_STREAM, (type == INET_SERVER) ? AI_PASSIVE : 0);
 
     /*
      * Maybe also accept a local address
      */
 
     if (type != INET_SERVER && (!NIL_P(arg->local.host) || !NIL_P(arg->local.serv))) {
-        arg->local.res = rsock_addrinfo(arg->local.host, arg->local.serv,
-                                        family, SOCK_STREAM, 0);
+        arg->local.res = rsock_addrinfo(arg->local.host, arg->local.serv, family, SOCK_STREAM, 0);
     }
 
     arg->fd = fd = -1;
+
+    VALUE io = Qnil;
+
     for (res = arg->remote.res->ai; res; res = res->ai_next) {
 #if !defined(INET6) && defined(AF_INET6)
         if (res->ai_family == AF_INET6)
@@ -94,19 +93,26 @@ init_inetsock_internal(VALUE v)
                 lres = arg->local.res->ai;
             }
         }
-        status = rsock_socket(res->ai_family,res->ai_socktype,res->ai_protocol);
+
+        status = rsock_socket(res->ai_family, res->ai_socktype, res->ai_protocol);
         syscall = "socket(2)";
         fd = status;
+
         if (fd < 0) {
             error = errno;
             continue;
         }
+
         arg->fd = fd;
+
+        VALUE io = rsock_init_sock(arg->sock, fd);
+        struct rb_io *fptr;
+        RB_IO_POINTER(io, fptr);
+
         if (type == INET_SERVER) {
 #if !defined(_WIN32) && !defined(__CYGWIN__)
             status = 1;
-            setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
-                       (char*)&status, (socklen_t)sizeof(status));
+            setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (char*)&status, (socklen_t)sizeof(status));
 #endif
             status = bind(fd, res->ai_addr, res->ai_addrlen);
             syscall = "bind(2)";
@@ -124,20 +130,21 @@ init_inetsock_internal(VALUE v)
             }
 
             if (status >= 0) {
-                status = rsock_connect(fd, res->ai_addr, res->ai_addrlen,
-                                       (type == INET_SOCKS), tv);
+                status = rsock_connect(io, res->ai_addr, res->ai_addrlen, (type == INET_SOCKS), tv);
                 syscall = "connect(2)";
             }
         }
 
         if (status < 0) {
             error = errno;
-            close(fd);
+            rb_io_close(io);
             arg->fd = fd = -1;
             continue;
-        } else
+        } else {
             break;
+        }
     }
+
     if (status < 0) {
         VALUE host, port;
 
@@ -158,19 +165,17 @@ init_inetsock_internal(VALUE v)
         status = listen(fd, SOMAXCONN);
         if (status < 0) {
             error = errno;
-            close(fd);
+            rb_io_close(io);
             rb_syserr_fail(error, "listen(2)");
         }
     }
 
     /* create new instance */
-    return rsock_init_sock(arg->sock, fd);
+    return io;
 }
 
 VALUE
-rsock_init_inetsock(VALUE sock, VALUE remote_host, VALUE remote_serv,
-                    VALUE local_host, VALUE local_serv, int type,
-                    VALUE resolv_timeout, VALUE connect_timeout)
+rsock_init_inetsock(VALUE sock, VALUE remote_host, VALUE remote_serv, VALUE local_host, VALUE local_serv, int type, VALUE resolv_timeout, VALUE connect_timeout)
 {
     struct inetsock_arg arg;
     arg.sock = sock;
@@ -225,9 +230,8 @@ ip_inspect(VALUE sock)
     union_sockaddr addr;
     socklen_t len = (socklen_t)sizeof addr;
     ID id;
-    if (fptr && fptr->fd >= 0 &&
-        getsockname(fptr->fd, &addr.addr, &len) >= 0 &&
-        (id = rsock_intern_family(addr.addr.sa_family)) != 0) {
+
+    if (fptr && fptr->fd >= 0 && getsockname(fptr->fd, &addr.addr, &len) >= 0 && (id = rsock_intern_family(addr.addr.sa_family)) != 0) {
         VALUE family = rb_id2str(id);
         char hbuf[1024], pbuf[1024];
         long slen = RSTRING_LEN(str);
@@ -236,8 +240,7 @@ ip_inspect(VALUE sock)
         str = rb_str_subseq(str, 0, slen);
         rb_str_cat_cstr(str, ", ");
         rb_str_append(str, family);
-        if (!rb_getnameinfo(&addr.addr, len, hbuf, sizeof(hbuf),
-                            pbuf, sizeof(pbuf), NI_NUMERICHOST | NI_NUMERICSERV)) {
+        if (!rb_getnameinfo(&addr.addr, len, hbuf, sizeof(hbuf), pbuf, sizeof(pbuf), NI_NUMERICHOST | NI_NUMERICSERV)) {
             rb_str_cat_cstr(str, ", ");
             rb_str_cat_cstr(str, hbuf);
             rb_str_cat_cstr(str, ", ");
@@ -274,16 +277,13 @@ ip_inspect(VALUE sock)
 static VALUE
 ip_addr(int argc, VALUE *argv, VALUE sock)
 {
-    rb_io_t *fptr;
     union_sockaddr addr;
     socklen_t len = (socklen_t)sizeof addr;
     int norevlookup;
 
-    GetOpenFile(sock, fptr);
-
     if (argc < 1 || !rsock_revlookup_flag(argv[0], &norevlookup))
-        norevlookup = fptr->mode & FMODE_NOREVLOOKUP;
-    if (getsockname(fptr->fd, &addr.addr, &len) < 0)
+        norevlookup = rb_io_mode(sock) & FMODE_NOREVLOOKUP;
+    if (getsockname(rb_io_descriptor(sock), &addr.addr, &len) < 0)
         rb_sys_fail("getsockname(2)");
     return rsock_ipaddr(&addr.addr, len, norevlookup);
 }
@@ -315,16 +315,13 @@ ip_addr(int argc, VALUE *argv, VALUE sock)
 static VALUE
 ip_peeraddr(int argc, VALUE *argv, VALUE sock)
 {
-    rb_io_t *fptr;
     union_sockaddr addr;
     socklen_t len = (socklen_t)sizeof addr;
     int norevlookup;
 
-    GetOpenFile(sock, fptr);
-
     if (argc < 1 || !rsock_revlookup_flag(argv[0], &norevlookup))
-        norevlookup = fptr->mode & FMODE_NOREVLOOKUP;
-    if (getpeername(fptr->fd, &addr.addr, &len) < 0)
+        norevlookup = rb_io_mode(sock) & FMODE_NOREVLOOKUP;
+    if (getpeername(rb_io_descriptor(sock), &addr.addr, &len) < 0)
         rb_sys_fail("getpeername(2)");
     return rsock_ipaddr(&addr.addr, len, norevlookup);
 }

ext/socket/rubysocket.h

@@ -296,8 +296,6 @@ int Rconnect();
 
 #include "constdefs.h"
 
-#define BLOCKING_REGION_FD(func, arg) (long)rb_thread_io_blocking_region((func), (arg), (arg)->fd)
-
 #define SockAddrStringValue(v) rsock_sockaddr_string_value(&(v))
 #define SockAddrStringValuePtr(v) rsock_sockaddr_string_value_ptr(&(v))
 #define SockAddrStringValueWithAddrinfo(v, rai_ret) rsock_sockaddr_string_value_with_addrinfo(&(v), &(rai_ret))
@@ -379,7 +377,7 @@ VALUE rsock_s_recvfrom_nonblock(VALUE sock, VALUE len, VALUE flg, VALUE str,
                                 VALUE ex, enum sock_recv_type from);
 VALUE rsock_s_recvfrom(VALUE sock, int argc, VALUE *argv, enum sock_recv_type from);
 
-int rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks, struct timeval *timeout);
+int rsock_connect(VALUE io, const struct sockaddr *sockaddr, int len, int socks, struct timeval *timeout);
 
 VALUE rsock_s_accept(VALUE klass, VALUE io, struct sockaddr *sockaddr, socklen_t *len);
 VALUE rsock_s_accept_nonblock(VALUE klass, VALUE ex, rb_io_t *fptr,

ext/socket/socket.c

@@ -391,13 +391,14 @@ sock_connect(VALUE sock, VALUE addr)
 {
     VALUE rai;
     rb_io_t *fptr;
-    int fd, n;
+    int n;
 
     SockAddrStringValueWithAddrinfo(addr, rai);
     addr = rb_str_new4(addr);
     GetOpenFile(sock, fptr);
-    fd = fptr->fd;
-    n = rsock_connect(fd, (struct sockaddr*)RSTRING_PTR(addr), RSTRING_SOCKLEN(addr), 0, NULL);
+
+    n = rsock_connect(sock, (struct sockaddr*)RSTRING_PTR(addr), RSTRING_SOCKLEN(addr), 0, NULL);
+
     if (n < 0) {
         rsock_sys_fail_raddrinfo_or_sockaddr("connect(2)", addr, rai);
     }

ext/socket/udpsocket.c

@@ -46,21 +46,17 @@ udp_init(int argc, VALUE *argv, VALUE sock)
 struct udp_arg
 {
     struct rb_addrinfo *res;
-    rb_io_t *fptr;
+    VALUE io;
 };
 
 static VALUE
 udp_connect_internal(VALUE v)
 {
     struct udp_arg *arg = (void *)v;
-    rb_io_t *fptr;
-    int fd;
     struct addrinfo *res;
 
-    rb_io_check_closed(fptr = arg->fptr);
-    fd = fptr->fd;
     for (res = arg->res->ai; res; res = res->ai_next) {
-        if (rsock_connect(fd, res->ai_addr, res->ai_addrlen, 0, NULL) >= 0) {
+        if (rsock_connect(arg->io, res->ai_addr, res->ai_addrlen, 0, NULL) >= 0) {
             return Qtrue;
         }
     }
@@ -87,10 +83,12 @@ static VALUE
 udp_connect(VALUE sock, VALUE host, VALUE port)
 {
     struct udp_arg arg;
+    rb_io_t *fptr;
     VALUE ret;
 
-    GetOpenFile(sock, arg.fptr);
-    arg.res = rsock_addrinfo(host, port, rsock_fd_family(arg.fptr->fd), SOCK_DGRAM, 0);
+    GetOpenFile(sock, fptr);
+    arg.res = rsock_addrinfo(host, port, rsock_fd_family(fptr->fd), SOCK_DGRAM, 0);
+    arg.io = sock;
     ret = rb_ensure(udp_connect_internal, (VALUE)&arg,
                     rsock_freeaddrinfo, (VALUE)arg.res);
     if (!ret) rsock_sys_fail_host_port("connect(2)", host, port);
@@ -105,7 +103,7 @@ udp_bind_internal(VALUE v)
     int fd;
     struct addrinfo *res;
 
-    rb_io_check_closed(fptr = arg->fptr);
+    GetOpenFile(arg->io, fptr);
     fd = fptr->fd;
     for (res = arg->res->ai; res; res = res->ai_next) {
         if (bind(fd, res->ai_addr, res->ai_addrlen) < 0) {
@@ -132,10 +130,12 @@ static VALUE
 udp_bind(VALUE sock, VALUE host, VALUE port)
 {
     struct udp_arg arg;
+    rb_io_t *fptr;
     VALUE ret;
 
-    GetOpenFile(sock, arg.fptr);
-    arg.res = rsock_addrinfo(host, port, rsock_fd_family(arg.fptr->fd), SOCK_DGRAM, 0);
+    GetOpenFile(sock, fptr);
+    arg.res = rsock_addrinfo(host, port, rsock_fd_family(fptr->fd), SOCK_DGRAM, 0);
+    arg.io = sock;
     ret = rb_ensure(udp_bind_internal, (VALUE)&arg,
                     rsock_freeaddrinfo, (VALUE)arg.res);
     if (!ret) rsock_sys_fail_host_port("bind(2)", host, port);
@@ -166,7 +166,7 @@ udp_send_internal(VALUE v)
         rb_io_wait(fptr->self, RB_INT2NUM(RUBY_IO_WRITABLE), Qnil);
 #endif
 
-        ssize_t n = (ssize_t)BLOCKING_REGION_FD(rsock_sendto_blocking, &arg->sarg);
+        ssize_t n = (ssize_t)rb_thread_io_blocking_region(fptr, rsock_sendto_blocking, &arg->sarg);
 
         if (n >= 0) return RB_SSIZE2NUM(n);