Feature #4532

[PATCH] add IO#pread and IO#pwrite methods

Added by Eric Wong about 3 years ago. Updated over 1 year ago.

[ruby-core:35566]
Status:Assigned
Priority:Normal
Assignee:Motohiro KOSAKI
Category:core
Target version:next minor

Description

=begin
These methods are useful for safe/concurrent file I/O in
multi-thread/process environments and also fairly standard
nowadays especially in systems supporting pthreads.

pread() is already used internally for IO.copy_stream
=end

0001-add-IO-pread-and-IO-pwrite-methods.patch Magnifier - patch with tests to implement feature (6.22 KB) Eric Wong, 03/28/2011 02:06 PM

History

#1 Updated by Motohiro KOSAKI about 3 years ago

=begin
2011/3/28 Eric Wong normalperson@yhbt.net:

Issue #4532 has been reported by Eric Wong.


Feature #4532: [PATCH] add IO#pread and IO#pwrite methods
http://redmine.ruby-lang.org/issues/4532

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x

These methods are useful for safe/concurrent file I/O in
multi-thread/process environments and also fairly standard
nowadays especially in systems supporting pthreads.

pread() is already used internally for IO.copy_stream

Do we really need to introduce new method? Why can't we overload
IO.read and IO.write?
too complex?

I agree "offset" argument is useful. But I'm not convinced this API
design is best. The description
is too quiet.

Ok, back to meta reviewing comments. All new API proposal need to
explain why this way
is best and need to persuade matz.
=end

#2 Updated by Eric Wong about 3 years ago

=begin
KOSAKI Motohiro kosaki.motohiro@gmail.com wrote:

2011/3/28 Eric Wong normalperson@yhbt.net:

Issue #4532 has been reported by Eric Wong.


Feature #4532: [PATCH] add IO#pread and IO#pwrite methods
http://redmine.ruby-lang.org/issues/4532

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x

These methods are useful for safe/concurrent file I/O in
multi-thread/process environments and also fairly standard
nowadays especially in systems supporting pthreads.

pread() is already used internally for IO.copy_stream

Do we really need to introduce new method? Why can't we overload
IO.read and IO.write?
too complex?

IO#read and IO#write take userspace buffers into account which
makes no sense with pread/pwrite.

I considered overloading IO#sysread and IO#syswrite, but it would be
hard for users to determine whether offset is supported on their
platform.

New methods means IO.methoddefined? and IO#respondto? to be used.

I'm not a fan of throwing NotImplementedError and faking with lseek() +
read()/write() to be even worse since it loses the atomicity guarantee.

I also considered putting the new methods in File instead of IO, but
sysseek is an IO method so I put it in IO.

I agree "offset" argument is useful. But I'm not convinced this API
design is best. The description
is too quiet.

Ok, back to meta reviewing comments. All new API proposal need to
explain why this way
is best and need to persuade matz.

Thanks again for your time!

--
Eric Wong
=end

#3 Updated by Motohiro KOSAKI about 3 years ago

=begin
2011/3/29 Eric Wong normalperson@yhbt.net:

KOSAKI Motohiro kosaki.motohiro@gmail.com wrote:

2011/3/28 Eric Wong normalperson@yhbt.net:

Issue #4532 has been reported by Eric Wong.


Feature #4532: [PATCH] add IO#pread and IO#pwrite methods
http://redmine.ruby-lang.org/issues/4532

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x

These methods are useful for safe/concurrent file I/O in
multi-thread/process environments and also fairly standard
nowadays especially in systems supporting pthreads.

pread() is already used internally for IO.copy_stream

Do we really need to introduce new method? Why can't we overload
IO.read and IO.write?
too complex?

IO#read and IO#write take userspace buffers into account which
makes no sense with pread/pwrite.

userspace buffer is implementation detail. no?

And, If pread is always behave as binary mode read method, your
documentation is much misleading. IMHO.

  • f = File.new("testfile")
  • f.pread(16, 0)   #=> "This is line one"
    

    I considered overloading IO#sysread and IO#syswrite, but it would be
    hard for users to determine whether offset is supported on their
    platform.

    ?? Why?
    Do you have any example?

    New methods means IO.methoddefined? and IO#respondto? to be used.

    OK, fair point.

    I'm not a fan of throwing NotImplementedError and faking with lseek() +
    read()/write() to be even worse since it loses the atomicity guarantee.

    I disagree. I dislike following part of your patch.

    #ifdef HAVEPREAD
    rb
    definemethod(rbcIO, "pread", rbiopread, -1);
    #endif

    This is very wrong style for new method. Eventually, all users need to call
    method_defined? before pread. Just NotImplementedError (or lseek emulation)
    makes much simpler script.

    I also considered putting the new methods in File instead of IO, but
    sysseek is an IO method so I put it in IO.

    I agree IO is better.
    =end

#4 Updated by Eric Wong about 3 years ago

=begin
KOSAKI Motohiro kosaki.motohiro@gmail.com wrote:

2011/3/29 Eric Wong normalperson@yhbt.net:

KOSAKI Motohiro kosaki.motohiro@gmail.com wrote:

Do we really need to introduce new method? Why can't we overload
IO.read and IO.write?
too complex?

IO#read and IO#write take userspace buffers into account which
makes no sense with pread/pwrite.

userspace buffer is implementation detail. no?

It's a very important detail. Different processes don't share userspace
buffers and Ruby should be able to share buffers with others (non-Ruby
processes), so we must only use the kernel cache.

And, If pread is always behave as binary mode read method, your
documentation is much misleading. IMHO.

  • f = File.new("testfile")
  • f.pread(16, 0) #=> "This is line one"

Yes, I just copied docs from sysread/syswrite :x I will update them
in the next patch I post.

I'm not a fan of throwing NotImplementedError and faking with lseek() +
read()/write() to be even worse since it loses the atomicity guarantee.

I disagree. I dislike following part of your patch.

#ifdef HAVEPREAD
rb
definemethod(rbcIO, "pread", rbiopread, -1);
#endif

This is very wrong style for new method. Eventually, all users need to call
method_defined? before pread. Just NotImplementedError (or lseek emulation)
makes much simpler script.

OK, shall I resubmit with adding optional offset argument to
sysread/syswrite and raise NotImplementedError?

Thank you again for your comments!

--
Eric Wong
=end

#5 Updated by Motohiro KOSAKI about 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Yukihiro Matsumoto

=begin
Hi

I personally still dislike pread/pwrite method name, but I give up to argue it because I'm worry about making endless discussion. Let's assign this ticket to matz and hear his opinions. :)

=end

#6 Updated by Koichi Sasada over 1 year ago

  • Description updated (diff)
  • Assignee changed from Yukihiro Matsumoto to Motohiro KOSAKI
  • Target version changed from 2.0.0 to next minor

kosaki-san, could you talk with matz about this ticket?
Please change target to 2.0 if you success to persuade matz.

Also available in: Atom PDF