773643 - [OS.File] (de)serialize errors

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

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

We need to transmit errors between threads.

Comment 1

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

Attachment: Shared code

Comment 2

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

Attachment: Companion testsuite

Comment 3

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

Attachment: Unix errors

Comment 4

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

Attachment: Windows errors

Comment 5

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

Attachment: Companion testsuite

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 649585 [details] [diff] [review]
Unix errors

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

Hardly anything to complain about!

::: toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ +136,5 @@
> +  /**
> +   * Deserialize a message back to an instance of OSError
> +   */
> +  OSError.fromMsg = function fromMsg(msg) {
> +    return new OSError(msg.operation, msg.unixErrno);

Are we doing epsilon error checking everywhere in the deserialization process?

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 649586 [details] [diff] [review]
Windows errors

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

::: toolkit/components/osfile/osfile_win_allthreads.jsm
@@ +138,5 @@
> +   */
> +  OSError.toMsg = function toMsg(error) {
> +    return {
> +      operation: error.operation,
> +     winLastError: error.winLastError

Nit: indentation issues.

Comment 8

[Nathan Froyd [:froydnj]]

I do wonder whether |toMsg| should be defined on the prototype, or whether that would represent a major break from all other |toMsg| functions elsewhere.

Comment 9

[Nathan Froyd [:froydnj]]

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

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js
@@ +98,5 @@
> +      check: function check_error(candidate, prefix) {
> +        ok(candidate instanceof OS.File.Error,
> +          prefix + "Error is an OS.File.Error");
> +        ok(candidate.unixErrno == 1 || candidate.winLastError == 1,
> +           prefix + "Error code is correct");

What do you think about providing an errorNumber getter for OS.File.Error so that we can avoid platform-specific details here?

Comment 10

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

(In reply to Nathan Froyd (:froydnj) from comment #6)
> Comment on attachment 649585 [details] [diff] [review]
> Unix errors
> 
> Review of attachment 649585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hardly anything to complain about!
> 
> ::: toolkit/components/osfile/osfile_unix_allthreads.jsm
> @@ +136,5 @@
> > +  /**
> > +   * Deserialize a message back to an instance of OSError
> > +   */
> > +  OSError.fromMsg = function fromMsg(msg) {
> > +    return new OSError(msg.operation, msg.unixErrno);
> 
> Are we doing epsilon error checking everywhere in the deserialization
> process?

I am not familiar with that term. What does this mean?

Comment 11

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

(In reply to Nathan Froyd (:froydnj) from comment #9)
> Comment on attachment 649587 [details] [diff] [review]
> Companion testsuite
> 
> Review of attachment 649587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js
> @@ +98,5 @@
> > +      check: function check_error(candidate, prefix) {
> > +        ok(candidate instanceof OS.File.Error,
> > +          prefix + "Error is an OS.File.Error");
> > +        ok(candidate.unixErrno == 1 || candidate.winLastError == 1,
> > +           prefix + "Error code is correct");
> 
> What do you think about providing an errorNumber getter for OS.File.Error so
> that we can avoid platform-specific details here?

Since the constants error number are platform-specific, I do not think there would be any gain. OSError.prototype offers a number of |becauseXXX| getters (which still need to be fleshed out a little) that can be used to portably determine the reason of an error.

Here, the arbitrary choice of "error 1" is just to keep the test simple.

Comment 12

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

(In reply to Nathan Froyd (:froydnj) from comment #8)
> I do wonder whether |toMsg| should be defined on the prototype, or whether
> that would represent a major break from all other |toMsg| functions
> elsewhere.

This is definitely possible. For many data structures, I have no choice, and I need |toMsg| to be a function rather than a method, either because I cannot extend the values or because I need to cope with |null|. For errors, I just kept it here for homogeneity.

I would rather maintain this homogeneity but I have no strong objection to changing this if you want me to.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller from comment #10)
> > ::: toolkit/components/osfile/osfile_unix_allthreads.jsm
> > @@ +136,5 @@
> > > +  /**
> > > +   * Deserialize a message back to an instance of OSError
> > > +   */
> > > +  OSError.fromMsg = function fromMsg(msg) {
> > > +    return new OSError(msg.operation, msg.unixErrno);
> > 
> > Are we doing epsilon error checking everywhere in the deserialization
> > process?
> 
> I am not familiar with that term. What does this mean?

Sorry, thought you might have been!  "epsilon" is the generic mathematical term for "a quantity arbitrarily close to zero"

(In reply to David Rajchenbach Teller from comment #11)
> > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js
> > @@ +98,5 @@
> > > +      check: function check_error(candidate, prefix) {
> > > +        ok(candidate instanceof OS.File.Error,
> > > +          prefix + "Error is an OS.File.Error");
> > > +        ok(candidate.unixErrno == 1 || candidate.winLastError == 1,
> > > +           prefix + "Error code is correct");
> > 
> > What do you think about providing an errorNumber getter for OS.File.Error so
> > that we can avoid platform-specific details here?
> 
> Since the constants error number are platform-specific, I do not think there
> would be any gain. OSError.prototype offers a number of |becauseXXX| getters
> (which still need to be fleshed out a little) that can be used to portably
> determine the reason of an error.
> 
> Here, the arbitrary choice of "error 1" is just to keep the test simple.

Indeed.  I was concerned about the (quite outside) possibility that the Unix code might define winLastError or vice versa.  But this is probably not a large enough detail to care about.

Comment 14

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

(In reply to Nathan Froyd (:froydnj) from comment #13)
> > > Are we doing epsilon error checking everywhere in the deserialization
> > > process?
> > 
> > I am not familiar with that term. What does this mean?
> 
> Sorry, thought you might have been!  "epsilon" is the generic mathematical
> term for "a quantity arbitrarily close to zero"

I am quite familiar with epsilon, just not with epsilon error checking :)
$$\forall x \in D, \forall y \in D, \forall \epsilon > 0, \exists \eta > 0, dist1(x, y) < \eta \Rightarrow dist2(f(x), f(y)) < \epsilon$$
maybe? :)


> > Since the constants error number are platform-specific, I do not think there
> > would be any gain. OSError.prototype offers a number of |becauseXXX| getters
> > (which still need to be fleshed out a little) that can be used to portably
> > determine the reason of an error.
> > 
> > Here, the arbitrary choice of "error 1" is just to keep the test simple.
> 
> Indeed.  I was concerned about the (quite outside) possibility that the Unix
> code might define winLastError or vice versa.  But this is probably not a
> large enough detail to care about.

ok

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller from comment #14)
> (In reply to Nathan Froyd (:froydnj) from comment #13)
> > > > Are we doing epsilon error checking everywhere in the deserialization
> > > > process?
> > > 
> > > I am not familiar with that term. What does this mean?
> > 
> > Sorry, thought you might have been!  "epsilon" is the generic mathematical
> > term for "a quantity arbitrarily close to zero"
> 
> I am quite familiar with epsilon, just not with epsilon error checking :)
> $$\forall x \in D, \forall y \in D, \forall \epsilon > 0, \exists \eta > 0,
> dist1(x, y) < \eta \Rightarrow dist2(f(x), f(y)) < \epsilon$$
> maybe? :)

Yes, something like that. :)  I guess I should have more clearly stated it as "an epsilon amount of error checking".

(In reply to David Rajchenbach Teller from comment #12)
> (In reply to Nathan Froyd (:froydnj) from comment #8)
> > I do wonder whether |toMsg| should be defined on the prototype, or whether
> > that would represent a major break from all other |toMsg| functions
> > elsewhere.
> 
> This is definitely possible. For many data structures, I have no choice, and
> I need |toMsg| to be a function rather than a method, either because I
> cannot extend the values or because I need to cope with |null|. For errors,
> I just kept it here for homogeneity.
> 
> I would rather maintain this homogeneity but I have no strong objection to
> changing this if you want me to.

Homogeneity is a good thing; if we can't make the switch in a majority of the places, let's just keep it as-is.

Comment 16

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

Attachment: Companion testsuite

Comment 17

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

Attachment: Windows errors

Comment 18

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

Attachment: Unix errors

Comment 19

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

try: https://tbpl.mozilla.org/?tree=Try&rev=52d842713958

Comment 20

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

Tweaking landing order.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Rajchenbach Teller from comment #19)
> try: https://tbpl.mozilla.org/?tree=Try&rev=52d842713958

Green on Try. Had to un-bitrot it a bit first. Please double-check to confirm that I didn't do anything dumb.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4387edee4099
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c71c9f71a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2ccd16a2c0

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4387edee4099
https://hg.mozilla.org/mozilla-central/rev/03c71c9f71a9
https://hg.mozilla.org/mozilla-central/rev/eb2ccd16a2c0

Comment 23

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

Ryan: looks good to me.