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

Convert to models after NSFetchedResultsController finishes reporting changes #3508

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

laevandus
Copy link
Contributor

@laevandus laevandus commented Nov 26, 2024

🔗 Issue Links

Resolves IOS-569

🎯 Goal

Fix a crash caused by infinite loop in Core Data when converting models

📝 Summary

  • Delay converting models to the point of time when NSFetchedRequestController has finished reporting changes

🛠 Implementation

We had an internal crash report which showed that Core Data change came in, triggered NSFetchedRequestController delegate, then we converted the DTO to a model type, but since the model type did another fetch request, this triggered the whole cycle again. Therefore, let's avoid it by delaying the model conversion to the point of time where NSFetchedRequestController has finished reporting changes.

🧪 Manual Testing Notes

Retrying the flow in IOS-569 which triggered it.

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • Changelog is updated with new localization keys
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

@laevandus laevandus requested a review from a team as a code owner November 26, 2024 08:50
@Stream-SDK-Bot
Copy link
Collaborator

SDK Size

title develop branch diff status
StreamChat 7.06 MB 7.06 MB 0 KB 🟢
StreamChatUI 4.96 MB 4.96 MB 0 KB 🟢

@laevandus laevandus added 🐞 Bug An issue or PR related to a bug 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK labels Nov 26, 2024
@Stream-SDK-Bot
Copy link
Collaborator

SDK Performance

target metric benchmark branch performance status
MessageList Hitches total duration 10 ms 3.34 ms 66.6% 🔼 🟢
Duration 2.6 s 2.55 s 1.92% 🔼 🟢
Hitch time ratio 4 ms per s 1.31 ms per s 67.25% 🔼 🟢
Frame rate 75 fps 78.11 fps 4.15% 🔼 🟢
Number of hitches 1 0.4 60.0% 🔼 🟢

@laevandus laevandus force-pushed the fix/core-data-update-cycle branch from 07eae5b to 629460a Compare November 26, 2024 08:59
@laevandus laevandus changed the title Convert to models after NSFetchResultsController finishes reporting changes Convert to models after NSFetchedResultsController finishes reporting changes Nov 26, 2024
… changes for avoiding update cycle triggered by fetch requests in item creator
@laevandus laevandus force-pushed the fix/core-data-update-cycle branch from 629460a to a1bc59e Compare November 26, 2024 09:01
@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Nov 26, 2024

SDK Size

title develop branch diff status
StreamChat 7.06 MB 7.06 MB 0 KB 🟢
StreamChatUI 4.96 MB 4.96 MB 0 KB 🟢

@laevandus laevandus added the 🤞 Ready For QA A PR that is Ready for QA label Nov 26, 2024
Comment on lines +222 to +223
// Model conversion is safe when all the changes have been processed (Core Data's _processRecentChanges can be called if conversion triggers additional fetch requests).
let itemChanges = currentChanges.compactMap { dtoChange in
Copy link
Member

@nuno-vieira nuno-vieira Nov 26, 2024

Choose a reason for hiding this comment

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

This makes sense. Does this potentially fix the current workaround we have with the depth stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no because the circular cycle can still occur with MessageDTO's replies contains a quoted message.

Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

Nice find 👌

@nuno-vieira
Copy link
Member

I've QA it myself, it seems to work fine 👍 At least I was not able to reproduce the crash anymore.

@nuno-vieira nuno-vieira added 🟢 QAed A PR that was QAed and removed 🤞 Ready For QA A PR that is Ready for QA labels Nov 26, 2024
@laevandus laevandus merged commit 82b28ef into develop Nov 27, 2024
13 checks passed
@laevandus laevandus deleted the fix/core-data-update-cycle branch November 27, 2024 07:52
@Stream-SDK-Bot Stream-SDK-Bot mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug An issue or PR related to a bug 🟢 QAed A PR that was QAed 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants