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

Replace Assertion Failure with Error for Premature Container Ends #356

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

rmarrowstone
Copy link
Contributor

This change replaces what is an assertion failure with a returned
error when the user attempts to step out of an Ion Container that
was not completed before the stream ended.

The issue is causing ion-python loads to hang for this case.

GH #355

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This change replaces what is an assertion failure with a returned
error when the user attempts to step out of an Ion Container that
was not completed before the stream ended.

The issue is causing ion-python loads to hang for this case.

GH #355
jobarr-amzn
jobarr-amzn previously approved these changes Jan 7, 2025
@rmarrowstone
Copy link
Contributor Author

Update: I cross-checked this behavior with the text reader. Apparently the text reader raises on error on reader_next. I will add that as well.

jobarr-amzn
jobarr-amzn previously approved these changes Jan 7, 2025
@rmarrowstone
Copy link
Contributor Author

I'm going to need to revisit this as my change appears to be causing build failures.

I realized the text reader also throws an error when the user tries
to get the next value from within the container. That seems right
to me as well, so I added a check there and then a test to prove
consistency.
@rmarrowstone rmarrowstone deleted the premature-container-end-fix branch January 7, 2025 20:20
@rmarrowstone rmarrowstone restored the premature-container-end-fix branch January 7, 2025 20:20
@rmarrowstone rmarrowstone reopened this Jan 7, 2025
@rmarrowstone rmarrowstone force-pushed the premature-container-end-fix branch from e5e5f1e to 6332473 Compare January 7, 2025 20:21
@rmarrowstone rmarrowstone merged commit d61c09a into master Jan 9, 2025
10 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants