> Subject: [PATCH] io.c (rb_io_close): ensure IOError for cross-thread clos=
es
>
> 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
> ---
> =A0io.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 25 ++++++++++++++++++------=
-
> =A0test/ruby/test_io.rb | =A0 39 +++++++++++++++++++++++++++++++++++++++
> =A02 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)
> =A0 =A0 if (keepgvl)
> =A0 =A0 =A0 =A0return close(fd);
>
> + =A0 =A0rb_thread_fd_close(fd);
> =A0 =A0 /*
> =A0 =A0 =A0* close() may block for certain file types (NFS, SO_LINGER soc=
kets,
> =A0 =A0 =A0* inotify), so let other threads run.
> @@ -3525,6 +3526,8 @@ maygvl_fclose(FILE *file, int keepgvl)
> =A0 =A0 if (keepgvl)
> =A0 =A0 =A0 =A0return fclose(file);
>
> + =A0 =A0rb_thread_fd_close(fileno(file));
> +
> =A0 =A0 return (int)rb_thread_blocking_region(nogvl_fclose, file, RUBY_UB=
F_IO, 0);
> =A0}
>
> @@ -3555,24 +3558,35 @@ fptr_finalize(rb_io_t *fptr, int noraise)
> =A0 =A0 =A0 =A0}
> =A0 =A0 }
> =A0 =A0 if (IS_PREP_STDIO(fptr) || fptr->fd <=3D 2) {
> + =A0 =A0 =A0 =A0int fd =3D fptr->fd;
> +
> + =A0 =A0 =A0 =A0fptr->stdio_file =3D 0;
> + =A0 =A0 =A0 =A0fptr->fd =3D -1;
> + =A0 =A0 =A0 =A0rb_thread_fd_close(fd);
> =A0 =A0 =A0 =A0 goto skip_fd_close;
> =A0 =A0 }
> =A0 =A0 if (fptr->stdio_file) {
> + =A0 =A0 =A0 FILE *stdio_file =3D fptr->stdio_file;
> +
> + =A0 =A0 =A0 fptr->stdio_file =3D 0;
> + =A0 =A0 =A0 fptr->fd =3D -1;
> +
> =A0 =A0 =A0 =A0 /* fptr->stdio_file is deallocated anyway
> =A0 =A0 =A0 =A0 =A0* even if fclose failed. =A0*/
> - =A0 =A0 =A0 if ((maygvl_fclose(fptr->stdio_file, noraise) < 0) && NIL_P=
(err))
> + =A0 =A0 =A0 if ((maygvl_fclose(stdio_file, noraise) < 0) && NIL_P(err))
> =A0 =A0 =A0 =A0 =A0 =A0 err =3D noraise ? Qtrue : INT2NUM(errno);
> =A0 =A0 }
> =A0 =A0 else if (0 <=3D fptr->fd) {
> + =A0 =A0 =A0 int fd =3D fptr->fd;
> + =A0 =A0 =A0 fptr->fd =3D -1;
> +
> =A0 =A0 =A0 =A0 /* fptr->fd may be closed even if close fails.
> =A0 =A0 =A0 =A0 =A0* POSIX doesn't specify it.
> =A0 =A0 =A0 =A0 =A0* We assumes it is closed. =A0*/
> - =A0 =A0 =A0 if ((maygvl_close(fptr->fd, noraise) < 0) && NIL_P(err))
> + =A0 =A0 =A0 if ((maygvl_close(fd, noraise) < 0) && NIL_P(err))
> =A0 =A0 =A0 =A0 =A0 =A0err =3D noraise ? Qtrue : INT2NUM(errno);
> =A0 =A0 }
> =A0 skip_fd_close:
> - =A0 =A0fptr->fd =3D -1;
> - =A0 =A0fptr->stdio_file =3D 0;
> =A0 =A0 fptr->mode &=3D ~(FMODE_READABLE|FMODE_WRITABLE);
>
> =A0 =A0 if (!NIL_P(err) && !noraise) {
> @@ -3668,7 +3682,6 @@ VALUE
> =A0rb_io_close(VALUE io)
> =A0{
> =A0 =A0 rb_io_t *fptr;
> - =A0 =A0int fd;
> =A0 =A0 VALUE write_io;
> =A0 =A0 rb_io_t *write_fptr;
>
> @@ -3684,9 +3697,7 @@ rb_io_close(VALUE io)
> =A0 =A0 if (!fptr) return Qnil;
> =A0 =A0 if (fptr->fd < 0) return Qnil;
>
> - =A0 =A0fd =3D fptr->fd;
> =A0 =A0 rb_io_fptr_cleanup(fptr, FALSE);
> - =A0 =A0rb_thread_fd_close(fd);
>
> =A0 =A0 if (fptr->pid) {
> =A0 =A0 =A0 =A0rb_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.