Enable relaxed error handling by default for modules #7245

mppf · 2017-09-08T16:53:12Z

Continuing PR #7244, this PR turns on relaxed error handling for more code and adjusts the modules and tests appropriately to match.

This PR adjusts several writeThis methods to use <~> instead of channel.write since the operator saves an error code in the channel instead of throwing. This is a workaround for the problem discussed in issue #7261.

This PR follows on earlier work in which the top-level proc writeln and associated routines (write, writef) do not throw. The idea is that they appear frequently enough & a program would be messed up enough if they didn't work that it should be a fatal error. In contrast, the methods on channels, such as proc channel.writeln() do throw since it's possible to encounter errors when doing file or network I/O.

Along the same lines, we considered possibly making string.format() not throw, but there is really only one name for this routine, and it's definitely less common than writeln.

full local testing

Reviewed by @psahabu - thanks!

@psahabu

Enable error mode selection per-module See issue #7055. This PR is the implementation portion of adding error handling mode selection on a per-module basis. Since we have not completed the language design in this area, it just uses flags for now, so that we can make progress on the implementation and issues in the module code. It enables 3 modes: * strict - same as strict was before this PR * relaxed - same as strict but you don't have to use 'try' or 'try!' on every throwing call * fatal - same as relaxed was before this PR (unmanaged errors result in halt) Since there is a lot of module/test code that has an issue with strict- or relaxed- by default for module declarations, this PR temporarily disables that and makes everything default to fatal. PR #7245 will re-enable non-implicit modules being checked by default. Passed full local testing. Reviewed by @psahabu - thanks!

psahabu

Looks good to me. (Now I see what you mean by the complicated IO changes! Yikes.)

psahabu · 2017-09-12T00:27:39Z

modules/packages/RecordParser.chpl

     :arg t: the record type to read
     :arg myReader: the channel to read from
   */
-  proc RecordReader(type t, myReader) {
+  proc RecordReader(type t, myReader) throws {


Have we done a throws on a constructor? Not opposed to it if you think it's fine, just curious.

No, but it seems to pass testing. I plan to turn it in to try! if anything goes wrong.

(as it should be, it was disabled as a workaround)

<~> saves the error in the channel, unlike write which currently throws.

These implicitly use try!. The idea is that they are common enough and errors in them are strange enough.

While there, removed an unused function.

bradcray · 2017-09-12T17:33:38Z

test/io/ferguson/writef.chpl

@@ -1,3 +1,4 @@
+pragma "error mode fatal"


I was just reviewing this to see what tests needed to be updated with this switch. Is it the use of writef()s in this test that requires a switch or something else?

Actually this one doesn't really need the switch (it did in an earlier version of the PR, when I had the top-level writef throw - it does not now).

bradcray · 2017-09-12T17:37:26Z

test/release/examples/primers/opaque.chpl

@@ -7,6 +7,7 @@
   support unstructured arrays like graphs. They are a special case of
   :ref:`associative domains and arrays <primers-associative>`.
 */
+pragma "error mode fatal"


It seems unfortunate to require the pragma in release/examples tests, and particularly in a primer. In this specific case, I wonder if the explicit module declaration could/should just be removed (most primers don't seem to use them unless they're teaching modules?). In other cases (benchmarks), I'm wondering whether try! could/should be used instead?

Looking at the rendered version of this online makes me think even more that we should just remove the module declaration in this case. It doesn't seem to have a purpose.

I think it's a matter of file naming. We don't want the implicit module to be named opaque because that might interfere with things like domain(opaque);. If we're willing to rename it to opaque-domains or something like that, I think we can drop the module declaration.

In updating it, I'd pull the module declaration anyway I think. It just clutters the documentation.

I have PR #7277 to fix this about the opaque primer.

Sorry, I got my wires crossed: I'd seen your "this doesn't need the pragma" comment on email and thought it was on this comment stream rather than the previous one (which is why my last response probably didn't parse well). Bummer about the need to dodge the implicit module name, but I'm OK with that PR.

The comment that kicked this stream off was also intended to wonder about what could be done to avoid using the pragma in other release/examples tests as well—I just attached it to this one since it was one of the most user-facing.

I can get rid of the test/release/examples/benchmarks/ssca2 ones by removing some module declarations (so we instead use the implicit module). Arguably the module statements make it clearer what is going on, but I think it'd be acceptable to remove them and rely on the implicit default. Sound OK?

I think this sounds OK. If the modified versions in the studies directory is also a performance test, I might do the same there in order to keep it compiling with older compilers. If not, leaving it as-is is fine.

See PR #7287.

bradcray · 2017-09-12T17:55:13Z

test/release/examples/benchmarks/lcals/LCALSMain.chpl

@@ -101,7 +101,7 @@ module main {
    run_loop[LoopKernelID.FIND_FIRST_MIN] = runC_findFirstMin;

    if output_dirname.length > 0 then
-      mkdir(output_dirname, parents=true);
+      try! mkdir(output_dirname, parents=true);


Having just run release-over-release performance gathering, it occurs to me that a downside of this edit (which I liked on first blush due to its precision) is that it will prevent getting performance on this test for past releases of Chapel. :(

This wouldn't be much better, but is something I've been curious about for other reasons: Do we still have a global compiler flag that can be used to dial down strictness? How does it play w.r.t. the pragma?

We don't currently have a global compiler flag to dial down the strictness. Recall that before, we had only --strict-errors and --no-strict-errors, where --no-strict-errors was the global default. But I removed that flag with my earlier PR. We could certainly add a flag for --fatal-errors e.g.. Do you think we should?

Also, if we want this test to work release-over-release, we could just have it call the out error= version of mkdir and include its own halt call. That should work the same before & after all the error handling changes.

Thanks for the info (I'd lost track of the flag). W.r.t. adding a flag: I don't feel certain about it, but while musing over the "fine-grain control" conversation yesterday and possible usage modes, it seemed likely that one might be "I'm going to use explicit modules, but I really don't want to be strict about my error-handling (yet?) and don't want to have to use a pragma on each module," which might suggest supporting such a compiler flag.

I'm not sure whether --fatal-errors is the right name for it or not. Intuitively, it seems to me that the flag would be saying something like "explicit modules are relaxed w.r.t. error-handling requirements." But this may be another conversation that suggests "Brad needs to refresh his understanding of the three modes."

Also, if we want this test to work release-over-release, we could just have it call the out error= version of mkdir and include its own halt call.

That might be nice, but is it a temporary solution? (i.e., are we planning on deprecating those interfaces in a future release?)

It's not exactly only compile-time or run-time, because it's a choice between compile-time failure for the code that doesn't handle errors vs. run-time error checking. I think it is different runtime behavior - checking for errors and halting if any are found. That is different run-time behavior than you'd get with marking a function 'throws' say (which is probably the most common way to consume the errors).

Other potential flag names: --lazy-errors or --prototype-errors

Of the ones we've discussed, --lazy-errors is most appealing to me in its conciseness and the broad net it casts. Though it's not as concise, I also find --enable-lazy-errors or --permit-lazy-errors to be slightly clearer. Might be time to get some opinions other than mine on this question.

Apart from name, is the feature easy to implement? (I'm guessing so). Is it correct to characterize that it would not change the behavior of anything that is making an effort to handle errors properly and successfully does so?

Yes - easy to implement and also wouldn't change the behavior of anything making an effort to handle errors properly. And I think I'd implement it to just change the default for modules from "error mode relaxed" to "error mode fatal".

That sounds right to me. You're likely already on this, but I'd implement it, and then send out a mail or issue explaining it and containing a short poll with favorite names to see if there's a clear winner.

Continuation/bug-fix of chapel-lang#7245. That PR adjusted several `writeThis` methods to use `<~>` instead of `channel.write` since the operator saves an error code in the channel instead of throwing. This was a workaround for the problem discussed in issue chapel-lang#7261. This makes the same change for network atomics, which was missed in that PR.

Fix writeThis in network atomics module Continuation/bug-fix of #7245. That PR adjusted several `writeThis` methods to use `<~>` instead of `channel.write` since the operator saves an error code in the channel instead of throwing. This was a workaround for the problem discussed in issue #7261. This makes the same change for network atomics, which was missed in that PR.

mppf added the stat: Work in Progress label Sep 8, 2017

mppf mentioned this pull request Sep 8, 2017

Enable error mode selection per-module #7244

Merged

mppf force-pushed the enable-strict-default branch from cd899c6 to b676c08 Compare September 11, 2017 14:35

mppf mentioned this pull request Sep 11, 2017

error handling: returning errors from writeThis #7261

Closed

mppf force-pushed the enable-strict-default branch 2 times, most recently from 9bef9b9 to 188979c Compare September 11, 2017 20:43

psahabu self-requested a review September 12, 2017 00:24

psahabu approved these changes Sep 12, 2017

View reviewed changes

psahabu mentioned this pull request Sep 12, 2017

error handling: modes and fine-grained selection #7055

Closed

mppf force-pushed the enable-strict-default branch from 188979c to 3f4e356 Compare September 12, 2017 14:35

mppf added 19 commits September 12, 2017 11:32

Add 'throws' to more I/O methods

ae0add8

Improve format() error handling

8f53b7b

Make the default for modules be ERROR_MODE_RELAXED

c171ee3

(as it should be, it was disabled as a workaround)

Change writes in Atomics writeThis to <~>

1b941c6

<~> saves the error in the channel, unlike write which currently throws.

Ignore error handing compiler errors in this module

83687cf

Tidy up error handling for writeBytes, assertEOF

0cbe265

Handle errors in readBytes/writeBytes

1d85ee1

Use <~> instead of f.write in DefaultSparse dsiSerialWrite

abe32c6

Tidy up error handling in Buffers.chpl

cb32505

Tidy up error handling in FileSystem.chpl

668b244

Use fatal errors in these tests with modules

9641abf

Use <~> instead of write in more writeThis methods

421b401

Make write/writeln/writef/string.format not throw

1e7621a

These implicitly use try!. The idea is that they are common enough and errors in them are strange enough.

Use <~> in writeThis in ChapelIO

087750a

Adjust RecordParser for strict error handling

a5eb9dc

Adjust BigInteger for strict error handling

d00abe6

Use fatal errors in TOML.chpl for now

50c036f

Adjust Spawn for strict error handling

e65a206

While there, removed an unused function.

Fix strict error handling in IO: openreader,...

ba52093

mppf added 10 commits September 12, 2017 11:35

Tidy up error handling in FileSystem

51d7e57

Use fatal-error-mode for MatrixMarket package module

e5ef2c8

Use fatal error mode for this test

d9183d0

Make string.format() throw

3378991

in ChapelSyncvar, fix aligned_t writeThis

f706cc0

Handle errors in regexp writeThis/readThis

f21290b

Test updates to enable fatal error handing in certain cases

68c95b6

Use try! in command_line_cast so regexp errors work

b6878a6

More error fixes for tests

f78e304

Mark these test modules as errors-are-fatal

ac2f673

mppf force-pushed the enable-strict-default branch from 5cda3f6 to ac2f673 Compare September 12, 2017 15:39

mppf merged commit 4dbd714 into chapel-lang:master Sep 12, 2017

mppf deleted the enable-strict-default branch September 12, 2017 17:24

bradcray reviewed Sep 12, 2017

View reviewed changes

psahabu changed the title ~~Enable strict error handling by default for modules~~ Enable relaxed error handling by default for modules Sep 12, 2017

bradcray reviewed Sep 12, 2017

View reviewed changes

mppf mentioned this pull request Sep 13, 2017

error handling: modes, compiler flag for release #7292

Closed

ronawho mentioned this pull request Sep 13, 2017

Fix writeThis in network atomics module #7293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable relaxed error handling by default for modules #7245

Enable relaxed error handling by default for modules #7245

mppf commented Sep 8, 2017 •

edited by psahabu

psahabu left a comment

psahabu Sep 12, 2017 •

edited

mppf Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 12, 2017

bradcray Sep 12, 2017

mppf Sep 12, 2017

mppf Sep 12, 2017

bradcray Sep 13, 2017

mppf Sep 13, 2017

bradcray Sep 13, 2017

Enable relaxed error handling by default for modules #7245

Enable relaxed error handling by default for modules #7245

Conversation

mppf commented Sep 8, 2017 • edited by psahabu

psahabu left a comment

Choose a reason for hiding this comment

psahabu Sep 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf commented Sep 8, 2017 •

edited by psahabu

psahabu Sep 12, 2017 •

edited