780483 - [OS.File] rename readAll -> read, read -> readOnce

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Nathan pointed out (quite fairly, I believe) that most users will not use functions read/write but rather readAll/writeAll.

To avoid bad surprises for users, we can do the following:
- as the only interesting characteristic of |read| wrt |readAll| is that it performs exactly one piece of I/O, rename the current |read| to |readOnce|;
- as |readAll| has the semantics that most users will expect, rename |readAll| into |read|;
- same thing for |write|/|writeAll|.

What do you think, Taras?

Comment 1

[(dormant account)]

(In reply to David Rajchenbach Teller from comment #0)
> Nathan pointed out (quite fairly, I believe) that most users will not use
> functions read/write but rather readAll/writeAll.
right

> 
> To avoid bad surprises for users, we can do the following:
> - as the only interesting characteristic of |read| wrt |readAll| is that it
> performs exactly one piece of I/O, rename the current |read| to |readOnce|;
> - as |readAll| has the semantics that most users will expect, rename
> |readAll| into |read|;
> - same thing for |write|/|writeAll|.
> 
> What do you think, Taras?

Are you just proposing changing names here? I don't think that makes sense. read() vs readAll matches behavior from other file apis.

I suspect .readAll itself may not be as commonly used as higherlevel abstractions such as readFile and atomicWriteFile.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #1)
> > To avoid bad surprises for users, we can do the following:
> > - as the only interesting characteristic of |read| wrt |readAll| is that it
> > performs exactly one piece of I/O, rename the current |read| to |readOnce|;
> > - as |readAll| has the semantics that most users will expect, rename
> > |readAll| into |read|;
> > - same thing for |write|/|writeAll|.
> > 
> > What do you think, Taras?
> 
> Are you just proposing changing names here? I don't think that makes sense.
> read() vs readAll matches behavior from other file apis.

This bug is for after readAll is implemented.  Very fine-grained bugs here.  Semantics post-this bug:

- read (readAll before this bug): Always read as much as possible of the specified amount.  Make multiple OS-level read calls if need be.  This is in line with read() functions in other high-level languages.
- readOnce (read before this bug): Thin wrapper around OS-level read.

It's not clear to me that readOnce is ever going to get used.  It would almost always be a mistake to use it instead of read.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

While I agree that readAll will be less commonly used than readFile, I believe that readOnce will (and should) be even less used. Consequently, making it the default read is probably a bad idea.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Well, basically rejected by Taras.

Never mind, I will probably end up cooking a high-level I/O library on top of OS.File at some point :)

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #1)
> (In reply to David Rajchenbach Teller from comment #0)
> > Nathan pointed out (quite fairly, I believe) that most users will not use
> > functions read/write but rather readAll/writeAll.
> right
> 
> > 
> > To avoid bad surprises for users, we can do the following:
> > - as the only interesting characteristic of |read| wrt |readAll| is that it
> > performs exactly one piece of I/O, rename the current |read| to |readOnce|;
> > - as |readAll| has the semantics that most users will expect, rename
> > |readAll| into |read|;
> > - same thing for |write|/|writeAll|.
> > 
> > What do you think, Taras?
> 
> Are you just proposing changing names here? I don't think that makes sense.
> read() vs readAll matches behavior from other file apis.

Actually, I am not sure about that. I believe that function |os.read| is our read, while method |read| of file is |readAll|.

> 
> I suspect .readAll itself may not be as commonly used as higherlevel
> abstractions such as readFile and atomicWriteFile.

Comment 6

[(dormant account)]

So, I thought about this some more. I'm gonna let you guys (David+Nathan) decide. I think my main objection is non-standard names. So if we were to copy python here and let .read() read the whole file and read(size) to read a chunk that's ok.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Let me suggest one possible API:
- readAll(options), returns {buffer, bytes}
  if options.buffer is not provided, buffer is allocated with size nbytes
  if options.nbytes is not provided, it is set to the total size of the file (or the number of bytes remaining in the file?)
  additional option

- readOnce(buffer, nbytes, options), returns {buffer, bytes}
  behaves exactly as our current read, except it returns an object instead of a number - we might make nbytes optional and extract the length from buffer.byteLength when nbytes is not provided

- writeAll(buffer), returns bytes
  if nbytes is not provided, it is set to buffer.byteLength

- writeOnce(buffer, nbytes, options), returns bytes
  behaves exactly as our current write

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #6)
> So, I thought about this some more. I'm gonna let you guys (David+Nathan)
> decide. I think my main objection is non-standard names. So if we were to
> copy python here and let .read() read the whole file and read(size) to read
> a chunk that's ok.

I have no problem with this.

(In reply to David Rajchenbach Teller from comment #7)
> Let me suggest one possible API:
> - readAll(options), returns {buffer, bytes}
>   if options.buffer is not provided, buffer is allocated with size nbytes
>   if options.nbytes is not provided, it is set to the total size of the file
> (or the number of bytes remaining in the file?)
>   additional option

I think trying to squish together the allocating API (returns a new buffer of so many bytes) and the non-allocating/destructive API (uses a provided buffer) is a mistake.  These should be separate functions.  I like having the non-allocating/destructive API, but I don't know that we need it straightaway.

> - writeAll(buffer), returns bytes
>   if nbytes is not provided, it is set to buffer.byteLength

Nit: I think you should be able to specify the offset as well as the length, but maybe that can be handled by views on the buffer?

I feel like a broken record, but I really don't think we care about the {read,write}Once functions.  They should not be included.  You get exactly the same behavior from their more robust read/write counterparts and there's no weird failure modes.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> (In reply to Taras Glek (:taras) from comment #6)
> > So, I thought about this some more. I'm gonna let you guys (David+Nathan)
> > decide. I think my main objection is non-standard names. So if we were to
> > copy python here and let .read() read the whole file and read(size) to read
> > a chunk that's ok.
> 
> I have no problem with this.
> 
> (In reply to David Rajchenbach Teller from comment #7)
> > Let me suggest one possible API:
> > - readAll(options), returns {buffer, bytes}
> >   if options.buffer is not provided, buffer is allocated with size nbytes
> >   if options.nbytes is not provided, it is set to the total size of the file
> > (or the number of bytes remaining in the file?)
> >   additional option
> 
> I think trying to squish together the allocating API (returns a new buffer
> of so many bytes) and the non-allocating/destructive API (uses a provided
> buffer) is a mistake.  These should be separate functions.  I like having
> the non-allocating/destructive API, but I don't know that we need it
> straightaway.

Well, until bug 720949 lands, I pretty much need my ArrayBuffers to be allocated by the controller thread for the async I/O, which in turns requires that I have access to a non-alloc API.

Also, given the low-level nature of the API, I believe that it makes complete sense to still provide access to the non-alloc function, even if they are not the default function.

> > - writeAll(buffer), returns bytes
> >   if nbytes is not provided, it is set to buffer.byteLength
> 
> Nit: I think you should be able to specify the offset as well as the length,
> but maybe that can be handled by views on the buffer?

Good point. Added to the latest implementation of writeAll.

> I feel like a broken record, but I really don't think we care about the
> {read,write}Once functions.  They should not be included.  You get exactly
> the same behavior from their more robust read/write counterparts and there's
> no weird failure modes.

As mentioned above, I am pretty sure we need the non-alloc functions.
As for the read-/write-only-once functions, given that their main use is implementing |copy|, which we have already done, I agree that we can remove them for the time being.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: rename readAll -> read, read -> readOnce

Performing renaming.
Note that _read and _write remain accessible, as we need them somewhat public for the implementation of read and write. We could do without, but this seems the most readable implementation.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 651781 [details] [diff] [review]
Companion testsuite

Review of attachment 651781 [details] [diff] [review]:
-----------------------------------------------------------------

Will need changes for readInto.  Cancelling.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 651780 [details] [diff] [review]
rename readAll -> read, read -> readOnce

Review of attachment 651780 [details] [diff] [review]:
-----------------------------------------------------------------

This patch will need updates but seems sound otherwise.  I think leaving _read and _write in is perfectly fine.  I'd like to review this again for documentation changes.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: rename readTo -> read, writeFrom -> write

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 652365 [details] [diff] [review]
rename readTo -> read, writeFrom -> write

Review of attachment 652365 [details] [diff] [review]:
-----------------------------------------------------------------

Wait, what happened to allocating-read?  I thought we had discussed that.  The bug title, patch description, and patch header line are not matching up very nicely...

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +96,1 @@
>         return throw_on_negative("read",

This string should now be "_read", correct?

@@ +115,1 @@
>         return throw_on_negative("write",

...and "_write"...

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +116,2 @@
>         // |gBytesReadPtr| is a pointer to |gBytesRead|.
>         throw_on_zero("read",

...and "_read"...

@@ +137,2 @@
>         // |gBytesWrittenPtr| is a pointer to |gBytesWritten|.
>         throw_on_zero("write",

...and "_write"?

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 652366 [details] [diff] [review]
Companion testsuite

Review of attachment 652366 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling while we get the actual renaming patch worked out.  I don't understand why anything in this test should be renamed to _{read,write}, though; the test should be looking at the external functions of the module.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

For errors, the error name is (generally) supposed to be the libc function name. This is not 100% followed, in particular when there is no libc function, but I intended to leave "read" and "write", as, in this case, there is.

For allocating-read, this is probably 

- we agreed that |read| and |write| should be methods that "do what you expect" (i.e. do not add corner-case failures when the disk or file system cannot handle/provide all bytes at once);
- you wanted to separate read-and-allocate from read-and-do-not-allocate.

This left me to decide which version ended up being |read| and which ended up being |readWithASuffix|. I went for read-and-do-not-allocate, because it was more symmetric to |write| than its sister method, but I could have done the opposite.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #17)
> Comment on attachment 652366 [details] [diff] [review]
> Companion testsuite
> 
> Review of attachment 652366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cancelling while we get the actual renaming patch worked out.  I don't
> understand why anything in this test should be renamed to _{read,write},
> though; the test should be looking at the external functions of the module.

Well, I do not think that there is anything wrong with checking that internal methods also work. That and I did not want to completely change the semantics of the test just for the sake of renaming two methods. We might do this in a followup bug, if there is interest in removing all uses of |_read|/|_write|, but this seemd a bit useless for this bug.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: rename readTo -> read, writeFrom -> write

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> > Cancelling while we get the actual renaming patch worked out.  I don't
> > understand why anything in this test should be renamed to _{read,write},
> > though; the test should be looking at the external functions of the module.
> 
> Well, I do not think that there is anything wrong with checking that
> internal methods also work. That and I did not want to completely change the
> semantics of the test just for the sake of renaming two methods. We might do
> this in a followup bug, if there is interest in removing all uses of
> |_read|/|_write|, but this seemd a bit useless for this bug.

Sorry for not responding to this earlier.  Using the new, smarter read shouldn't be changing the semantics of the test; if anything, it should be increasing the robustness of the test, right?

(In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> For errors, the error name is (generally) supposed to be the libc function
> name. This is not 100% followed, in particular when there is no libc
> function, but I intended to leave "read" and "write", as, in this case,
> there is.

OK, that's reasonable.

> This left me to decide which version ended up being |read| and which ended
> up being |readWithASuffix|. I went for read-and-do-not-allocate, because it
> was more symmetric to |write| than its sister method, but I could have done
> the opposite.

Ah, see, I thought we had decided that read-and-allocate got the shorter name (e.g. comment 6 and comment 7).  The concern for symmetry is appropriate, but we would not be the first to break the symmetry here, and I think giving destructive methods a longer name is a good idea (and has precedent of its own, certainly).

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 655131 [details] [diff] [review]
rename readTo -> read, writeFrom -> write

Review of attachment 655131 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 21 is really the comment that matters.  The ideal patch would:

- rename readAll to read
- rename writeFrom to write
- rename to OS-level bits to _-prefixed counterparts

Comment 23

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #21)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> > > Cancelling while we get the actual renaming patch worked out.  I don't
> > > understand why anything in this test should be renamed to _{read,write},
> > > though; the test should be looking at the external functions of the module.
> > 
> > Well, I do not think that there is anything wrong with checking that
> > internal methods also work. That and I did not want to completely change the
> > semantics of the test just for the sake of renaming two methods. We might do
> > this in a followup bug, if there is interest in removing all uses of
> > |_read|/|_write|, but this seemd a bit useless for this bug.
> 
> Sorry for not responding to this earlier.  Using the new, smarter read
> shouldn't be changing the semantics of the test; if anything, it should be
> increasing the robustness of the test, right?

Well, yes, but we just wouldn't be testing the same thing. Not a big deal, though, I'll just replace back the _read/_write back with read/write.

> 
> (In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> > For errors, the error name is (generally) supposed to be the libc function
> > name. This is not 100% followed, in particular when there is no libc
> > function, but I intended to leave "read" and "write", as, in this case,
> > there is.
> 
> OK, that's reasonable.
> 
> > This left me to decide which version ended up being |read| and which ended
> > up being |readWithASuffix|. I went for read-and-do-not-allocate, because it
> > was more symmetric to |write| than its sister method, but I could have done
> > the opposite.
> 
> Ah, see, I thought we had decided that read-and-allocate got the shorter
> name (e.g. comment 6 and comment 7).  The concern for symmetry is
> appropriate, but we would not be the first to break the symmetry here, and I
> think giving destructive methods a longer name is a good idea (and has
> precedent of its own, certainly).

As both choices are meaningful, I will stop insisting :)

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: rename readAll -> read, writeFrom -> write, keep readTo

Here we go.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: rename readAll -> read, writeFrom -> write, keep readTo

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=d34e2d017c19

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #28)
> https://tbpl.mozilla.org/?tree=Try&rev=d34e2d017c19

"Green" on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/beb52f8dccd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0468b3f2f0c4

Comment 30

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/beb52f8dccd5
https://hg.mozilla.org/mozilla-central/rev/0468b3f2f0c4