[ruby-core:35702] Re: [Ruby 1.9 - Bug #4558] TestSocket#test_closed_read fails after r31230

> Subject: [PATCH] io.c (rb_io_close): ensure IOError for cross-thread closes
>
> We need to inform threads to stop operations on the FD before
> closing it and also invalidate the fd member of the rb_io_t
> struct for other threads to properly raise IOError.
>
> FDs may be created and destroyed without the GVL, so
> rb_thread_fd_close() may be improperly hitting the wrong
> threads/FDs if we close() before notifying and in the worst case
> or threads will end up reading/writing to an unexpected FD.
>
> ref: [ruby-core:35631]
> ref: http://redmine.ruby-lang.org/issues/4558
> ---
> o.c | 25 ++++++++++++++++++-------
> est/ruby/test_io.rb | 39 +++++++++++++++++++++++++++++++++++++++
>  files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/io.c b/io.c
> index 7ce7148..5d37b7f 100644
> --- a/io.c
> +++ b/io.c
> @@ -3504,6 +3504,7 @@ maygvl_close(int fd, int keepgvl)
> if (keepgvl)
> eturn close(fd);
>
> + b_thread_fd_close(fd);
> /*
>  close() may block for certain file types (NFS, SO_LINGER sockets,
>  inotify), so let other threads run.
> @@ -3525,6 +3526,8 @@ maygvl_fclose(FILE *file, int keepgvl)
> if (keepgvl)
> eturn fclose(file);
>
> + b_thread_fd_close(fileno(file));
> +
> return (int)rb_thread_blocking_region(nogvl_fclose, file, RUBY_UBF_IO, 0);
> 
>
> @@ -3555,24 +3558,35 @@ fptr_finalize(rb_io_t *fptr, int noraise)
> 
> }
> if (IS_PREP_STDIO(fptr) || fptr->fd <= 2) {
> + nt fd = fptr->fd;
> +
> + ptr->stdio_file = 0;
> + ptr->fd = -1;
> + b_thread_fd_close(fd);
> goto skip_fd_close;
> }
> if (fptr->stdio_file) {
> + FILE *stdio_file = fptr->stdio_file;
> +
> + fptr->stdio_file = 0;
> + fptr->fd = -1;
> +
> /* fptr->stdio_file is deallocated anyway
>  even if fclose failed. /
> - if ((maygvl_fclose(fptr->stdio_file, noraise) < 0) && NIL_P(err))
> + if ((maygvl_fclose(stdio_file, noraise) < 0) && NIL_P(err))
> err = noraise ? Qtrue : INT2NUM(errno);
> }
> else if (0 <= fptr->fd) {
> + int fd = fptr->fd;
> + fptr->fd = -1;
> +
> /* fptr->fd may be closed even if close fails.
>  POSIX doesn't specify it.
>  We assumes it is closed. /
> - if ((maygvl_close(fptr->fd, noraise) < 0) && NIL_P(err))
> + if ((maygvl_close(fd, noraise) < 0) && NIL_P(err))
> rr = noraise ? Qtrue : INT2NUM(errno);
> }
> skip_fd_close:
> - ptr->fd = -1;
> - ptr->stdio_file = 0;
> fptr->mode &= ~(FMODE_READABLE|FMODE_WRITABLE);
>
> if (!NIL_P(err) && !noraise) {
> @@ -3668,7 +3682,6 @@ VALUE
> b_io_close(VALUE io)
> 
> rb_io_t *fptr;
> - nt fd;
> VALUE write_io;
> rb_io_t *write_fptr;
>
> @@ -3684,9 +3697,7 @@ rb_io_close(VALUE io)
> if (!fptr) return Qnil;
> if (fptr->fd < 0) return Qnil;
>
> - d = fptr->fd;
> rb_io_fptr_cleanup(fptr, FALSE);
> - b_thread_fd_close(fd);
>
> if (fptr->pid) {
> b_syswait(fptr->pid);

No. Please no.
The original issue is not important one. This complex and ugly patch is no good
tradeoff for very small performance improvement.

I'll revert r31230. If close performance is very important for you, we need more
cleaner code.