[ruby-core:35429] Re: [Ruby 1.9 - Bug #4463][Open] [PATCH] release GVL for fcntl() for operations that may block

KOSAKI Motohiro <[e-mail]> wrote:
> Umm..
> I don't like its interface so much. your flock object don't mange any lock
> state. it's merely wrapper of argument of fcntl. your interface mean we need
> two line every lock operation. eg.
> 
>       lock = Fcntl::Flock.new Fcntl::F_WRLCK
>       f.fcntl Fcntl::F_SETLKW, lock

I agree it's currently too verbose.

I tried to keep io.c the same so I used a String subclass.  Maybe I
should just modify teach io.c to deal with Hash/Array arguments?  I do
worry about placing more burden on io.c for portability reasons, though
POSIX file locks might be very common by now...

To shorten interface, maybe Fcntl::Flock[] can return an array for splat
and take symbol args (like new Socket):

	f.fcntl *Fcntl::Flock[:F_SETLKW, :F_WRLCK, :SEEK_SET, 0, 0]

Or maybe even:

	f.fcntl *Fcntl::Flock[:SETLKW, :WRLCK, :SET, 0, 0]

> but I *personally* prefer array or hash capsulation. e.g
> 
>    f.fcntl Fcntl:F_SETLKW, [Fcntl:F_WRLK, SEEK_SET, 0, 0]
> 
>    or
> 
>    f.fcntl Fcntl:F_SETLKW, { :l_type => Fcntl:F_WRLK }

Yes, I like the Hash one but requires modifying io.c with potentially
unportable code to support.

If we use non-String, maybe just call fcntl(2) inside ext/fcntl/fcntl.c
internally and forget about IO#fcntl in io.c entirely:

	Fcntl::Flock[:WRLCK, :SET, 0, 0].lock(io)
	Fcntl::Flock[:WRLCK, :SET, 0, 0].try_lock(io)
	Fcntl::Flock[:SET, 0, 0].unlock(io)

Or even:

	Fcntl.lock(io, :WRLCK, :SET, 0, 0)
	Fcntl.try_lock(io, :WRLCK, :SET, 0, 0)
	Fcntl.unlock(io, :SET, 0, 0)
	Fcntl.getlock(io, :RDLCK, :SET, 0, 0) -> Fcntl::Flock object

That would allow us to do something stateful like:

	Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do
	  # ...
	end

I dislike all caps, even, taking hints from pthread_rwlock_*:

	Fcntl.rdlock(io, :set, 0, 0)
	Fcntl.tryrdlock(io, :set, 0, 0)
	Fcntl.wrlock(io, :set, 0, 0)
	Fcntl.trywrlock(io, :set, 0, 0)
	Fcntl.unlock(io, :set, 0, 0)

	Fcntl.read_synchronize(io, :set, 0, 0) do
	  # ...
	end

	Fcntl.synchronize(io, :set, 0, 0) do
	  # ...
	end

> But, of cource, I'm not against if matz ack yours. So I recommend you
> describe the detailed interface to matz instead only just attached a patch.
> It's best practice to persuade _very_ busy person. :)

Thanks again for the feedback.  So many ways to do this interface,
but just anything but Array#pack sounds good to me :)

-- 
Eric Wong