Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

error handling: returning errors from writeThis #7261

Closed
mppf opened this issue Sep 11, 2017 · 5 comments
Closed

error handling: returning errors from writeThis #7261

mppf opened this issue Sep 11, 2017 · 5 comments
Assignees
@dlongnecke-cray
Labels
area: Language area: Libraries / Modules type: Feature Request

Comments

@mppf
Copy link
Member

mppf commented Sep 11, 2017

The writeThis, readWriteThis, and readThis methods can be provided on a type to implement how that type is written or read.

As part of the error handling effort for Chapel 1.16, many I/O functions now are marked throws. Some of these might be called by the above methods like writeThis.

Currently, if there is an error performing I/O in support of writeThis/..., an error code is saved in the relevant channel. Then, subsequent attempts to do I/O on that channel as part of writeThis will instead do nothing and preserve the error.

This method of saving an error code in the channel doesn't fit with the throws style errors that we'd like to be able to use, however. In particular, one might imagine wanting to have a writeThis/... that can throw an arbitrary user error.

At the time of this writing, channel.writef or channel.write are marked throws, but the I/O operator <~> is not. Instead, the I/O operator always saves the "error" state in to the channel. For this reason, PR #7245 changes several calls to write to calls to <~> as a workaround for this issue. Longer term though, I think we want <~> to be equivalent to channel.write.

We could:

  1. make writeThis/... throws, and save the error state in the channel record. This strategy has the advantage of allowing arbitrary exceptions to be thrown by writeThis/..
  2. make writeThis/... optionally return an error code
  3. expect writeThis/... authors to manage explicitly setting an error code on the channel if an error is encountered
@psahabu psahabu mentioned this issue Sep 11, 2017
Closed
87 tasks
@mppf mppf mentioned this issue Sep 11, 2017
Merged
1 task
@mppf mppf changed the title returning errors from writeThis error handling: returning errors from writeThis Sep 12, 2017
mppf added a commit that referenced this issue Sep 12, 2017
@mppf
Merge pull request #7245 from mppf/enable-strict-default
4dbd714 
Enable strict error handling by default for modules

Continuing PR #7244, this PR turns on strict 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`.

- [x] full local testing

Reviewed by @psahabu - thanks!
ronawho added a commit to ronawho/chapel that referenced this issue Sep 13, 2017
@ronawho
Fix writeThis in network atomics module
572438c 
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.
@ronawho ronawho mentioned this issue Sep 13, 2017
Merged
ronawho added a commit that referenced this issue Sep 13, 2017
@ronawho
Merge pull request #7293 from ronawho/fix-net-atomic-writeThis
6b7ccb5 
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 mppf mentioned this issue Sep 13, 2017
Merged
5 tasks
mppf added a commit that referenced this issue Sep 13, 2017
@mppf
Merge pull request #7294 from mppf/fix-strict-failures
5de3a60 
Fix error handling compiler errors from nightly testing

Fixes error-handling related compilation errors from nightly testing and
applies the `<~>` workaround  for the problem described in #7261 in more
places in module code and in test code we might like to be module code soon.

Trivial and not reviewed.

- [x] verified that test/modules/packages/blas passes
- [x] verified that fifo deadlock detection/report works again
- [x] test/distributions with quickstart
- [x] test/distributions/robust/arithmetic/trivial/test_writeln with default, block, cyclic, replicated
- [x] full local testing
@mppf mppf mentioned this issue Dec 6, 2017
Open
16 tasks
@ronawho ronawho mentioned this issue Apr 27, 2018
Closed
15 tasks
This was referenced Jul 11, 2018
Open
Open
@BryantLam
Copy link

BryantLam commented Dec 5, 2018 •
edited
Loading

Just ran into this again (because I keep forgetting). Tried to do the equivalent of try! f <~> data which resulted in an understandably unhelpful compile error: syntax error: near <~>.

@mppf mppf mentioned this issue Jun 4, 2019
Merged
@BryantLam
Copy link

BryantLam commented Sep 24, 2019 •
edited
Loading

  1. expect writeThis/... authors to manage explicitly setting an error code on the channel if an error is encountered

This is the status quo today and I haven't internalized the implications until now. Using the <~> operator seems somewhat problematic in that if the channel ever hits an error, all subsequent writes to it silently fail. (RIght?..) While I've not seen stdout/stderr fail before, I won't rule it out in an e.g., long-running job that suddenly stops writing to stdout/stderr/some log or data output file vs. throwing an exception which has to be handled (but maybe the application is safe even if some kinds of writes silently fail?..).

@mppf
Copy link
Member Author

mppf commented Sep 24, 2019

Using the <~> operator seems somewhat problematic in that if the channel ever hits an error, all subsequent writes to it silently fail

It's possible to clear the error. Also I'm not sure I'd call it "silently failing". It's just within a write call that the error is hidden, and I/O calls ignored, until the write call retrieves the saved error and then throws.

In other words, the current status quo puts off handling the error until the outmost write call doing a writeThis completes; at which point it throws. It does that today with an error code, but in the future it could save an Error object. I think it could do it whether or not other I/O calls are ignored while the error is set.

@dlongnecke-cray dlongnecke-cray self-assigned this Oct 22, 2019
@mppf
Copy link
Member Author

mppf commented Jan 31, 2020

@dlongnecke-cray - is this one done now?

@dlongnecke-cray
Copy link
Contributor

Resolved by merge of #14739.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dlongnecke-cray dlongnecke-cray

Labels
area: Language area: Libraries / Modules type: Feature Request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mppf @BryantLam @dlongnecke-cray