Page MenuHomePhabricator
Differential D5234

Use GlobalDBSingleton when touching database from the native iOS code.
ClosedPublic
Actions

Authored by marcin on Sep 27 2022, 4:53 AM.
Tags
None
Referenced Files
F3369215: D5234.id17411.diff
Mon, Nov 25, 10:15 PM
F3369214: D5234.id17398.diff
Mon, Nov 25, 10:15 PM
F3369212: D5234.id17343.diff
Mon, Nov 25, 10:14 PM
F3369211: D5234.id17337.diff
Mon, Nov 25, 10:14 PM
F3369210: D5234.id17103.diff
Mon, Nov 25, 10:14 PM
F3369208: D5234.id.diff
Mon, Nov 25, 10:14 PM
F3369206: D5234.diff
Mon, Nov 25, 10:14 PM
F3368423: D5234.id17403.diff
Mon, Nov 25, 7:47 PM
View All Files
Subscribers
• abosh
ashoat

Details

Reviewers
tomek
atul
• jon
Commits
rCOMMc53645b26950: Use GlobalDBSingleton when touching database from the native iOS code.
Summary

This differential replaces any direct database access in iOS native code with GlobalDBSingleton API

Test Plan

Creating test release with those changes would remove iOS 137 crash

Diff Detail

Repository
rCOMM Comm
Branch
marcin/db-singleton
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Sep 27 2022, 4:53 AM
Herald added subscribers: • abosh, ashoat. · View Herald TranscriptSep 27 2022, 4:53 AM
marcin added a parent revision: D5233: Implement GlobalDbSingleton API for both platforms.Sep 27 2022, 4:54 AM
marcin retitled this revision from Use GlobalDBSingletonIOSProxy when touching database from the antive code. to Use GlobalDBSingletonIOSProxy when touching database from the native iOS code..
Harbormaster completed remote builds in B12456: Diff 17103.Sep 27 2022, 5:01 AM
marcin requested review of this revision.Sep 27 2022, 5:01 AM
• jon accepted this revision.Sep 27 2022, 9:18 AM
This revision is now accepted and ready to land.Sep 27 2022, 9:18 AM
marcin updated this revision to Diff 17168.Sep 29 2022, 1:46 AM

GlobalDBSingletonISOProxy API changed so updating AppDelegate.

Harbormaster completed remote builds in B12508: Diff 17168.Sep 29 2022, 1:55 AM
ashoat added a comment.Sep 29 2022, 7:21 AM

We should be really sure about this code. If there are any issues, it will probably cause crashes we don't know about. I encourage everybody on this diff to be extremely thorough. We should be 100% sure of every line here. We should not be guessing, or trying things that seem to work without understanding 100% of why/how they work.

tomek accepted this revision.Oct 3 2022, 6:13 AM

This is ok assuming we're using the proxy. I'm not yet convinced D5233 that this is the right thing to do, but this diff seems to be ok.

marcin updated this revision to Diff 17337.Oct 5 2022, 2:50 AM

Redefine API

marcin removed a parent revision: D5233: Implement GlobalDbSingleton API for both platforms.Oct 5 2022, 2:58 AM
marcin added a parent revision: D5232: Modify commCoreModule to use GlobalDBSingleton instead of creating its own thread.
marcin retitled this revision from Use GlobalDBSingletonIOSProxy when touching database from the native iOS code. to Use GlobalDBSingleton when touching database from the native iOS code..
marcin edited the summary of this revision. (Show Details)
Harbormaster failed remote builds in B12613: Diff 17337!Oct 5 2022, 3:00 AM
marcin updated this revision to Diff 17343.Oct 5 2022, 3:11 AM

Fix compilation bug

Harbormaster completed remote builds in B12619: Diff 17343.Oct 5 2022, 3:21 AM
ashoat added inline comments.Oct 6 2022, 5:33 AM
native/ios/Comm/AppDelegate.mm
171 ↗(On Diff #17343)

How sure are we that this doesn't need to be within GlobalDBSingleton? Looking into it, it seems like a couple assignments to instance variables, which should be safe... but we need to make sure there are no assignments from other threads (only one thread should ever call attemptDatabaseInitialization and SQLiteQueryExecutor::initialize), and we should be sure that there are no risks from reading from the variable while the assignment is in progress (if there are, we could consider an atomic variable or something)

marcin added inline comments.Oct 6 2022, 5:54 AM
native/ios/Comm/AppDelegate.mm
171 ↗(On Diff #17343)

attemptDatabaseInitialization and SQLiteQueryExecutor::initialize can be called from any thread since those method do not touch database and use std::call_once semantics which makes them idempotent (after it is called for the first time subsequent calls are no-ops) and thread safe.

ashoat added inline comments.Oct 6 2022, 6:19 AM
native/ios/Comm/AppDelegate.mm
171 ↗(On Diff #17343)

Makes sense, thanks!

tomek accepted this revision.Oct 6 2022, 6:55 AM
marcin updated this revision to Diff 17398.Oct 7 2022, 2:06 AM

Rebase before landing

Harbormaster completed remote builds in B12663: Diff 17398.Oct 7 2022, 2:17 AM
marcin updated this revision to Diff 17403.Oct 7 2022, 2:32 AM

Rebase before landing

Harbormaster completed remote builds in B12667: Diff 17403.Oct 7 2022, 2:42 AM
Closed by commit rCOMMc53645b26950: Use GlobalDBSingleton when touching database from the native iOS code. (authored by marcin). · Explain WhyOct 7 2022, 2:51 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMc53645b26950: Use GlobalDBSingleton when touching database from the native iOS code..

Revision Contents
Changeset List

PathSize
native/
ios/
Comm/
11 lines

Diff 17103

View Options

native/ios/Comm/AppDelegate.mm

Loading...