Project

General

Profile

Actions

Feature #4532

closed

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

Added by normalperson (Eric Wong) about 13 years ago. Updated almost 7 years ago.

Status:
Closed
Target version:
[ruby-core:35566]

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


Files

0001-add-IO-pread-and-IO-pwrite-methods.patch (6.22 KB) 0001-add-IO-pread-and-IO-pwrite-methods.patch patch with tests to implement feature normalperson (Eric Wong), 03/28/2011 02:06 PM
0001-Add-IO-pread-and-IO-pwrite-methods.patch (6.8 KB) 0001-Add-IO-pread-and-IO-pwrite-methods.patch Updated patch, for current trunk (57443) avsej (Sergey Avseyev), 01/27/2017 08:29 PM
0001-Add-IO-pread-and-IO-pwrite-methods-v3.patch (6.98 KB) 0001-Add-IO-pread-and-IO-pwrite-methods-v3.patch avsej (Sergey Avseyev), 01/27/2017 08:52 PM

Updated by kosaki (Motohiro KOSAKI) about 13 years ago

2011/3/28 Eric Wong :

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.

Updated by normalperson (Eric Wong) almost 13 years ago

KOSAKI Motohiro wrote:

2011/3/28 Eric Wong :

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

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

2011/3/29 Eric Wong :

KOSAKI Motohiro wrote:

2011/3/28 Eric Wong :

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.

Updated by normalperson (Eric Wong) almost 13 years ago

KOSAKI Motohiro wrote:

2011/3/29 Eric Wong :

KOSAKI Motohiro 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

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

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

Updated by ko1 (Koichi Sasada) over 11 years ago

  • 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.

Updated by avsej (Sergey Avseyev) about 7 years ago

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

Updated by avsej (Sergey Avseyev) about 7 years ago

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

Updated by avsej (Sergey Avseyev) about 7 years ago

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

The same patch as above, but with typo fixes

Actions #10

Updated by avsej (Sergey Avseyev) about 7 years ago

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

Updated by matz (Yukihiro Matsumoto) about 7 years ago

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

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

Matz.

Actions #13

Updated by nobu (Nobuyoshi Nakada) almost 7 years ago

  • 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 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0