Bug in documentation

[mortenbekditlevsen]

Describe the bug

I'm not certain if this is the correct place to report this, but the Amplify v2 Optimistic UI example at: https://docs.amplify.aws/swift/build-a-backend/data/optimistic-ui/#complete-example
is quite buggy.

It uses an actor to protect mutable state, but it carries values derived from that state across suspension points, introducing bugs that can cause crashes.

For instance the updateProperty function reads an index and a rollbackProperty, then awaits an API call (allowing other modification of the state to occur, for instance someone deleting all properties).
Then after the await, the index is used without re-validating, so in case the properties array has a size change, the index can now be invalid.

Example that can cause a crash:

1. call createProperty and then updateProperty with the same property - in an offline situation, so all API calls will fail
2. createProperty runs first and properties now has a count of 1
3. The Amplify API is called and suspends execution of createProperty
4. updateProperty runs and stores the value 0 in index
5. The Amplify API is called and suspends execution of updateProperty
6. createProperty resumes execution, but due to being offline we have an error, and we are removing the item at index 0 of the properties array - leaving an empty array. The createProperty function completes
7. The updateProperty resumes, also with an error, it attempts to update properties at index 0 which now doesn't exist, and it causes a crash.

Steps To Reproduce

This is a documentation issue.
Read https://docs.amplify.aws/swift/build-a-backend/data/optimistic-ui/#complete-example thoroughly and consider Swift actor re-entrancy.

Expected behavior

Documentation should not suggest anti-patterns that may cause crashes. :-)

Amplify Framework Version

2

Amplify Categories

API

Dependency manager

Swift PM

Swift version

5.10

CLI version

Xcode version

14.3.1

Relevant log output

<details>
<summary>Log Messages</summary>


INSERT LOG MESSAGES HERE

```

Is this a regression?

No

Regression additional context

No response

Platforms

No response

OS Version

iOS 15.3

Device

iPhone 15

Specific to simulators

No response

Additional context

No response

[harsh62]

This definitely is the right place. We will look into the issue and provide an udpate.

[mortenbekditlevsen]

Awesome! 😊
Let me know if I can do anything to help. You should be able to find relevant information by searching for 'Swift actor reentrancy' and look at the classic bank account example. 😊

[harsh62]

@mortenbekditlevsen
The docs are hosted at https://github.com/aws-amplify/docs .. Contribution are always welcomed. If you wanna take a stab at updating the doc, will be glad to review and merge it.

[mortenbekditlevsen]

@harsh62
Hi there,
I've been thinking about the code sample in your documentation quite a bit.
Fixing the crashers by re-validating indices after any suspension point is doable, but the logic would still be broken.

For instance two subsequent updates of a value first from '1' to '2' and the other from '2' to '3'. If both fail, and they both fail in order, then the latter 'roll back' would roll back to '2'.
So I'm afraid that I consider the implementation of the optimistic changes too fragile, while a version that perhaps implemented change tracking more accurately would likely make the example too complex and hard for new users to understand.
Apologies for suggesting that I could help, now that I'm bailing out. 😊

[harsh62]

Thanks. Our team will look into prioritizing this soon.