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.

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

error handling: non-inlined iterators cannot throw #7134

Open
4 tasks
mppf opened this issue Aug 27, 2017 · 2 comments
Open
4 tasks

error handling: non-inlined iterators cannot throw #7134

mppf opened this issue Aug 27, 2017 · 2 comments

Comments

@mppf
Copy link
Member

mppf commented Aug 27, 2017

As a Chapel user, I would like to be able to write iterators that throw whether they are inlined or not because I expect to be able to combine language features to achieve my goals. Additionally I would like library supported iterators, such as FileSystem.listdir, to throw errors so that I can handle them appropriately in my programs and libraries.

See for example

test/errhandling/ferguson/it-throws-noinline.chpl

At the present time, only iterators that are inlined can throw. Otherwise the compiler generates an error because this code path is not implemented completely yet. This issue is a place to refer to this problem.

Perhaps the iterator lowering process should make 'throwing' those functions built from the iterator (advance, zip1, zip2, etc) that contain code that was setting the error argument in the original iterator.

Because of this issue, some standard library iterators that should be marked throws instead halt on error:

  • ItemReader.these() (used in file.lines())
  • channel.matches()
  • RecordReader.stream_num() and RecordReader.stream()
  • FileSystem listdir()

Tests that are skipif-ed for nightly testing with --baseline include:

test/library/packages/csv/CSVtest.chpl

These should be updated to throw once this issue is resolved.

mppf added a commit to mppf/chapel that referenced this issue Aug 27, 2017
Issue chapel-lang#7134 indicates that throwing iterators don't currently
work unless they are inlined. That means that any iterator that is
used in tests in --baseline testing can't be marked throws without
causing errors in that configuration.

To quiet testing, this PR simply makes the two iter functions in
the standard modules that throw currently not throwing. Instead, these
functions use try! to halt if any error was encountered. This is
intended to be a temporary change.
mppf added a commit that referenced this issue Aug 27, 2017
For now, don't mark standard module iters as throws

Issue #7134 indicates that throwing iterators don't currently work unless
they are inlined. That means that any iterator that is used in tests in
--baseline testing can't be marked throws without causing errors in that
configuration.

To quiet testing, this PR simply makes the two throwing iter functions in
the standard modules no longer throw. Instead, these functions use try!
to halt if any error was encountered. This is intended to be a temporary
change.

- [x] full --baseline testing
- [x] full local testing

Reviewed by @psahabu - thanks!
@ronawho
Copy link
Contributor

ronawho commented Jun 8, 2018

Considering that iterator inlining is an optimization that can be disabled with --baseline, or --no-optimize-loop-iterators --no-inline-iterators I don't think we should add error-handling to any iterator until this is implemented.

And note that some iterators can be direct inlined (--inline-iterators), but cannot be zippered inlined (--optimize-loop-iterators) since the limitations on the iterator for the 2 inlining methods are different.

@mppf
Copy link
Member Author

mppf commented Jun 8, 2018

@ronawho - right, we havn't been adding error handling to iterators in the modules for this reason. (edit - at least as far as I know).

@ronawho ronawho added the Epic label Jun 11, 2018
ronawho added a commit that referenced this issue Jun 11, 2018
Add halt wrappers for use in initializers and iterators

[reviewed by @mppf]

Error handling isn't supported for initializers or iterators today. These halt
wrappers are intended to mark such cases so that we can more easily
find/identify them in the future.

See #7134 and 
#8793 for more info
mppf added a commit to mppf/chapel that referenced this issue Aug 9, 2018
This is a workaround for issue chapel-lang#7134.
PR chapel-lang#10642 got the compiler to consider loop-exprs to be `throwing`
if they call code that throws. As a result, these loop-exprs
are now iterators that throw, which are not supported in --baseline
testing.
mppf added a commit that referenced this issue Aug 9, 2018
Add .skipifs for baseline mode testing for several tests

This is a workaround for issue #7134.
PR #10642 got the compiler to consider loop-exprs to be `throwing`
if they call code that throws. As a result, these loop-exprs
are now iterators that throw, which are not supported in --baseline
testing.

Passed full baseline testing.
Trivial and not reviewed.
dlongnecke-cray added a commit that referenced this issue Jan 8, 2020
…14430)

Make the <~> operator equivalent to `channel.read`/`channel.write`
(#14430)

This PR makes the <~> IO operator throw, and insofar as possible
equivalent in semantics to `channel.read` and `channel.write`.

There are some unavoidable differences between the two - i.e., the
`channel.read` routine opts to return `false` if it hits the end
of a file, instead of throwing a SystemError.

If a SystemError is thrown from within IO routines up through
`readThis`/`writeThis`, then the error is caught and the equivalent
QIO error code is stored in the channel. If the user throws an
error that is not a SystemError from within their overload of
`readThis`/`writeThis`, the EIO error code is stored in the channel
as a stopgap to indicate an error has occurred.

As of this time there is no way to distinguish between SystemError
thrown by the user and SystemError that are thrown by the Chapel
IO machinery.

This PR adds the `throws` keyword to all the `dsi` IO routines for
arrays, as a consequence of modifying <~>.

The test `test/classes/deitz/class/tree.chpl` was rewritten to use
an iterative postorder traversal instead of a recursive iterator
due to compilation bugs. See #7134 for details.

---

Testing:

- [x] ALL on linux64 when CHPL_COMM=none
- [x] ALL on linux64 when CHPL_COMM=gasnet

---

Thanks to @mppf for review!
ben-albrecht pushed a commit to ben-albrecht/chapel that referenced this issue Mar 5, 2020
ben-albrecht added a commit that referenced this issue Mar 5, 2020
Remove throws from Itertools iterators 

Remove throws from Itertools iterators until #7134 is resolved. Resolves baseline failures in nightly testing.

- [x] local testing
- [x] local baseline testing

[Reviewed by @ronawho]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants