Feature #4532: [PATCH] add IO#pread and IO#pwrite methods

• Status: Closed
• Assignee: nobu (Nobuyoshi Nakada)
• Target version: 2.6

[normalperson (Eric Wong)]

=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

[kosaki (Motohiro KOSAKI)]

2011/3/28 Eric Wong [e-mail]:

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.

[normalperson (Eric Wong)]

KOSAKI Motohiro [e-mail] wrote:

2011/3/28 Eric Wong [e-mail]:

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.method_defined? and IO#respond_to? 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

[kosaki (Motohiro KOSAKI)]

2011/3/29 Eric Wong [e-mail]:

KOSAKI Motohiro [e-mail] wrote:

2011/3/28 Eric Wong [e-mail]:

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.method_defined? and IO#respond_to? 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 HAVE_PREAD
rb_define_method(rb_cIO, "pread", rb_io_pread, -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.

[normalperson (Eric Wong)]

KOSAKI Motohiro [e-mail] wrote:

2011/3/29 Eric Wong [e-mail]:

KOSAKI Motohiro [e-mail] 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 HAVE_PREAD
rb_define_method(rb_cIO, "pread", rb_io_pread, -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

[kosaki (Motohiro KOSAKI)]

• Status changed from Open to Assigned
• Assignee set to matz (Yukihiro Matsumoto)

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. :)

[ko1 (Koichi Sasada)]

• Description updated (diff)
• Assignee changed from matz (Yukihiro Matsumoto) to kosaki (Motohiro KOSAKI)
• Target version changed from 2.0.0 to 2.6

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

[avsej (Sergey Avseyev)]

Hi everyone, any plans to include pread/pwrite in near future?

[avsej (Sergey Avseyev)]

• File 0001-Add-IO-pread-and-IO-pwrite-methods.patch 0001-Add-IO-pread-and-IO-pwrite-methods.patch added

I rebased the patch against current trunk, and also made some improvements:

• raise NotImplementedError on platforms, which do not support pread/pwrite
• improved documentation
• fix argument order for IO#pwrite to be consistent with pwrite(2) and IO#pread, the offset should go last
• update tests and function names to follow the same style as other code

[avsej (Sergey Avseyev)]

• File 0001-Add-IO-pread-and-IO-pwrite-methods-v3.patch added

The same patch as above, but with typo fixes

[avsej (Sergey Avseyev)]

• File deleted (0001-Add-IO-pread-and-IO-pwrite-methods-v3.patch)

[avsej (Sergey Avseyev)]

• File 0001-Add-IO-pread-and-IO-pwrite-methods-v3.patch 0001-Add-IO-pread-and-IO-pwrite-methods-v3.patch added

[matz (Yukihiro Matsumoto)]

• Assignee changed from kosaki (Motohiro KOSAKI) to nobu (Nobuyoshi Nakada)

Agreed. pread/pwrite can be useful in some cases.

Matz.

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r58240.

---

Add IO#pread and IO#pwrite methods

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.

Based on patches by Avseyev [e-mail] at
[ruby-core:79290]. [Feature #4532]

• configure.in: check for pwrite(2). pread() is already used
  internally for IO.copy_stream.

• io.c: implement wrappers for pread(2) and pwrite(2) and expose
  them in IO.