Skip to content

fix(Datastore) : Use throwing iterator to avoid hidden force try #1411

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

Merged
merged 4 commits into from
Nov 2, 2021

Conversation

ashiemke
Copy link
Contributor

@ashiemke ashiemke commented Sep 3, 2021

Issue #, if available: None

Description of changes: Using the default iterator on a statement uses a force try, which crashes (although I'm not sure the circumstances). Replace that with a long-form while rather than for-in to re-throw exception rather than crashing.

See stephencelis/SQLite.swift#1030 for discussion

Check points:

- [ ] Added new tests to cover change, if needed

  • All unit tests pass
  • All integration tests pass
    - [ ] Documentation update for the change if required
  • PR title conforms to conventional commit style

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

ashiemke and others added 4 commits September 2, 2021 22:18
Using the default iterator on a statement uses a force try, which crashes (although I'm not sure the circumstances). Replace that with a long-form while rather than for-in to rethrow exception rather than crashing.
for-in creates an iterator before looping. Mimic that lifecycle
@ameter ameter added datastore Issues related to the DataStore category contribution Community contribution PRs labels Sep 3, 2021
@thisisabhash thisisabhash self-requested a review September 17, 2021 18:49
@thisisabhash
Copy link
Member

Hello, Could you please add information about when a situation would arise that'd cause a crash and possible ways to test it?

@ChadyG
Copy link

ChadyG commented Oct 13, 2021

@thisisabhash Sorry for the delay. I have a test that will usually generate this crash.

This can occur when calling DataStore.clear() during an initial sync after login. Our app calls clear on Auth signedOut events, so this pops up every so often. We have a test that logs in and back out a handful of times that will usually cause this crash to occur.

@ChadyG
Copy link

ChadyG commented Oct 25, 2021

@thisisabhash Any word on when this might get merged or updated?

@thisisabhash thisisabhash changed the title Use throwing iterator to avoid hidden force try fix(Datastore) : Use throwing iterator to avoid hidden force try Nov 1, 2021
Copy link
Member

@thisisabhash thisisabhash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing unit tests and integration tests are passing with the changes. LGTM.

@thisisabhash
Copy link
Member

SQLite codebase uses a force try in default iterator which causes a crash.
I tried to reproduce by calling Datastore.clear() on Auth signedOut events but couldn't reproduce.
Merging this in because it is difficult to replicate the crash in an integration test having a clear use case.
A unit test to cover the change is difficult as it is difficult to mock Statement and Connection classes because they are marked final and part of SQLite codebase.

Verified that existing integration tests and unit tests are passing for this PR.

@thisisabhash thisisabhash merged commit b902d3f into aws-amplify:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Community contribution PRs datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants