Skip to content

fix: Refactored computed property to avoid force-unwrap #86

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KristofferHermansen
Copy link

Description ✏️

We are encountering fatal errors caused by the force-unwrap used in the observers computed property.
This occurs due to a race condition where asynchronous code attempts to read the count of observers while some of the observers are being deallocated.
This issue was identified during unit tests, where it would occasionally cause random test failures.

Cause 🤔

The current implementation uses the filter function to ensure the map function can unwrap the optional observer property. However, this approach allows observers to be deallocated between the execution of these two functions. If deallocation occurs during this window, the force-unwrap results in a fatal error.

Solution 👨‍💻

The observers property has been refactored to use the compactMap function instead of filter and map. This eliminates the need for a force-unwrap and prevents the error.

Due to the timing-specific nature of the issue, I have not added unit tests to demonstrate the impact. However, after applying this change, the problem no longer occurs in our unit tests.

@KristofferHermansen
Copy link
Author

@artman we first observed this problem in our unit tests.
However we have recently started seeing crashes in our production apps.
The crash logs we have, all point back to the Signal.observers.getter as the cause of the crash.
Any chance you have time to take a look at this fix for the problem?

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.

1 participant