issue 1111 only initialize Realm and KeyChainService once #1117

tomholub · 2021-11-29T23:09:30Z

This PR cleans up realm init to minimize race conditions and have more control over when and how it's initialized.

close #1111

WIP

Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

tomholub · 2021-11-30T18:14:33Z

Sorry for another monstrous PR. The main goal of this PR was:

initialize Realm only once, and in a controlled manner
make the initialization a MainActor
pass initialized storage to places that need it (which turned out to be every single place of the app 😞 ) through dependency injection

For this I created AppContext that gets passed around.

While I was doing it, certain things kept getting in my way, in particular magical currentUser usage hidden inside classes/methods, which needed DataProvider which needed EncryptedStorage which needed that initialized Realm.. it was a mess, and so I also did some more work:

where the above situation was particularly upsetting, I've instead added currentUser as method or class parameter
I've done some prepwork for current user should be dependency-injected #863 by requiring user to be present early in VC init. Next step, for that issue, would be to pass User into each VC init that requires it.

While doing this, I noticed some classes are used across the app and rely on storage, and so I've added them into AppContext as well. Which has gotten bigger than I expected.

Things that bother me about this PR and will need to get cleaned up (hopefully in other PRs):

the dependency injection highlights an overall mess present across the app. There are too many services, providers, often named wrongly, often their constructor properties are named wrongly, etc. It was more hidden until now when you could just put dataProvider = DataProvider() in every constructor, but now that it's truly dependency-injected, it really badly brings out some ugliness that we need to work on
repetitive code. I have written some code sequences 5+times again and again while refactoring this, and it would need a second look
general verbosity. similar as point above, and needs to be improved
inconsitency. Sometimes it is user, sometimes activeUser, sometimes currentUser.. some abstractions are somewhat different on IMAP and Gmail, and need to be alighned. etc

As of now, the app builds and runs and I didn't run into a bug yet. But Swift tests are likely broken. I'll have a look.

tomholub

Some highlights below

FlowCrypt/App/AppContext.swift

tomholub · 2021-11-30T18:16:13Z

FlowCrypt/Controllers/CheckMailAuth/CheckMailAuthViewController.swift

 class CheckMailAuthViewController: TableNodeViewController {
    private let globalRouter: GlobalRouterType
+    private let appContext: AppContext

-    init(globalRouter: GlobalRouterType = GlobalRouter()) {
+    init(appContext: AppContext, globalRouter: GlobalRouterType = GlobalRouter()) {
+        self.appContext = appContext
        self.globalRouter = globalRouter
        super.init(node: TableNode())
    }


Context now gets passed as first argument of every VC init.

Next step will be to pass User (not optional) into every VC that requires it. #863

FlowCrypt/Controllers/Setup/SetupInitialViewController.swift

tomholub · 2021-11-30T18:20:25Z

FlowCrypt/Controllers/SideMenu/Menu/MyMenuViewController.swift

+        self.foldersProvider = foldersProvider ?? FoldersService(
+            encryptedStorage: appContext.encryptedStorage,
+            remoteFoldersProvider: appContext.getRequiredMailProvider().remoteFoldersProvider
+        )


I initialized FolderService many times this way, good candidate to go into AppContext (a method, maybe, similar to EmailProvider)

FlowCrypt/Functionality/DataManager/DataService.swift

FlowCrypt/Functionality/DataManager/Encrypted Storage/KeyChainService.swift

FlowCrypt/Functionality/Mail Provider/Gmail/GmailService.swift

tomholub · 2021-11-30T18:23:13Z

FlowCrypt/Functionality/Mail Provider/MailProvider.swift

-    private init(
-        currentAuthType: @autoclosure @escaping () -> (AuthType?),
-        services: [MailServiceProvider]
+    init(
+        currentAuthType: AuthType,
+        currentUser: User
    ) {
        self.currentAuthType = currentAuthType
-        self.services = services
+        self.services = MailServiceProviderFactory.services(user: currentUser)
    }


I was a bit worried about this - originally dynamic as a closure, now user is directly provided. Need to scrutinize more. But app seems to work the same.

FlowCrypt/Functionality/Services/GlobalRouter.swift

ivan-ushakov · 2021-11-30T18:55:13Z

Am I right that we have now one global context with app lifetime? What about Realm and threading? I see that context is forced to be created on UI thread, so Realms is created on that thread. That means we must do any read query on UI thread.

tomholub · 2021-11-30T18:58:08Z

Am I right that we have now one global context with app lifetime?

I believe so. (except that new AppContext gets created when session information changes - see AppContext.withSession but all other contents get copied over, including Realm / storage etc)

What about Realm and threading? I see that context is forced to be created on UI thread, so Realms is created on that thread. That means we must do any read query on UI thread.

That can be reworked - I don't mind to create Realm on any thread.

But the encryption key for realm, which uses KeyChain, must be created on main thread, to be safe.

I don't exactly mind sticking to main thread for Realm either, though. (if it doesn't cause issues, not sure)

tomholub · 2021-11-30T19:45:07Z

@ivan-ushakov could you please help me with these when running tests. I'm not very familiar with this stack.

ivan-ushakov · 2021-11-30T19:51:11Z

@ivan-ushakov could you please help me with these when running tests. I'm not very familiar with this stack.

Sure, let me check.

tomholub · 2021-11-30T19:52:37Z

I'm still hoarding some changes here. I'll go ahead and push them

ivan-ushakov · 2021-11-30T20:00:13Z

FlowCrypt/App/main.swift

@@ -9,16 +9,9 @@
 import Foundation
 import UIKit

-autoreleasepool {


Not sure we could skip autoreleasepool here

I'm not sure either - the IDE was telling me that it's no longer needed. @sosnovsky may know

ivan-ushakov · 2021-11-30T20:06:15Z

I'm still hoarding some changes here. I'll go ahead and push them

Looks like you added some UI classes to unit test target FlowCryptAppTests. We don't compile UI classes for unit tests so we don't add some Pod dependencies. I guess that this is the reason.

You could check Target Membership on the right side panel and remove checkmark for FlowCryptAppTests on UI files.

ivan-ushakov · 2021-12-01T05:18:04Z

@ivan-ushakov I managed to break it again. Although this time there aren't so many files in test target.

What steps did you take to fix this last time? So that I can learn it too.

git checkout master FlowCrypt.xcodeproj/project.pbxproj
Fix several errors like missing files, links to removed files. Xcode will help here.

sosnovsky · 2021-12-01T08:46:52Z

I'd still like to be using custom arguments in the future - for appium tests mocks

These arguments can be parsed in AppDelegate using CommandLine.arguments array, no need for custom main.swift

tomholub · 2021-12-01T10:23:00Z

These arguments can be parsed in AppDelegate using CommandLine.arguments array, no need for custom main.swift

Got it! Filed #1130

sosnovsky · 2021-12-01T10:24:01Z

@ivan-ushakov I managed to break it again. Although this time there aren't so many files in test target.

What steps did you take to fix this last time? So that I can learn it too.

Current issues are caused by https://github.com/FlowCrypt/flowcrypt-ios/blob/issue-1111-realm-keychain-init/FlowCryptAppTests/Functionality/Services/ComposeMessageServiceTests.swift#L31 - this code needs CoreHost which imports SwiftyRSA framework, but it's not included in test target.

sosnovsky · 2021-12-01T10:25:19Z

I tried to remove CoreHost there, but then I got error about missing CoreHost in Core.swift file, trying to fix it now

tomholub · 2021-12-01T10:27:14Z

The Core parts are needed for tests in either case. And so is SwiftyRSA.

tomholub · 2021-12-01T10:27:20Z

Thanks for help!

sosnovsky · 2021-12-01T10:27:41Z

Also for testing we can use in-memory Realm instance, instead of on-disk - it should be faster and doesn't need to be resetted, as it's new instance for each new test (https://docs.mongodb.com/realm/sdk/ios/test-and-debug/#testing)

tomholub · 2021-12-01T10:28:34Z

I'm not entirely sure we use Realm in Swift tests. But will file an issue.

sosnovsky · 2021-12-01T10:30:18Z

The Core parts are needed for tests in either case. And so is SwiftyRSA.

Yes, this Core.swift code is 1 month old and previously worked well, so it's strange why it doesn't run after the latest updates.

sosnovsky · 2021-12-01T10:31:54Z

I'm not entirely sure we use Realm in Swift tests. But will file an issue.

It's initiated only in ComposeMessageServiceTests - https://github.com/FlowCrypt/flowcrypt-ios/blob/issue-1111-realm-keychain-init/FlowCryptAppTests/Functionality/Services/ComposeMessageServiceTests.swift#L36
But maybe later we'll need it in other places too

sosnovsky · 2021-12-01T11:59:34Z

Currently unit tests are running, but ComposeMessageServiceTests fail, probably because of some Realm issues in test environment (as previously ComposeMessageService didn't have encryptedStorage in init)

tomholub · 2021-12-01T12:26:02Z

Currently unit tests are running, but ComposeMessageServiceTests fail, probably because of some Realm issues in test environment (as previously ComposeMessageService didn't have encryptedStorage in init)

Yes but one of its dependencies must have previously still needed Realm, even if Realm wasn't touched in the tests.

Do you want to look into this? I'm ok to temporarily disable those tests, too.

sosnovsky · 2021-12-01T12:27:39Z

Currently unit tests are running, but ComposeMessageServiceTests fail, probably because of some Realm issues in test environment (as previously ComposeMessageService didn't have encryptedStorage in init)

Yes but one of its dependencies must have previously still needed Realm, even if Realm wasn't touched in the tests.

Do you want to look into this? I'm ok to temporarily disable those tests, too.

Currently looking into this issue

sosnovsky · 2021-12-01T15:07:14Z

All tests are passing now ✅

tomholub · 2021-12-01T15:12:07Z

I'll merge this. After that, I'll review the already merged PR - there are many opportunities for small fixes. Then can settle that after. Thanks!

tomholub

Below are smaller cleanup/refactors that I wanted to do in this PR

tomholub · 2021-12-01T11:29:46Z

FlowCrypt/Controllers/Setup/SetupBackupsViewController.swift

        fetchedEncryptedKeys: [KeyDetails],
        router: GlobalRouterType = GlobalRouter(),
-        keyStorage: KeyStorageType = KeyDataStorage(),
        decorator: SetupViewDecorator = SetupViewDecorator(),
        core: Core = Core.shared,


Controllers don't need core in init, since we're not unit testing controllers

tomholub · 2021-12-01T11:29:59Z

FlowCrypt/Controllers/Setup/SetupBackupsViewController.swift

@@ -44,23 +43,21 @@ final class SetupBackupsViewController: TableNodeViewController, PassPhraseSavea
    }

    init(
+        appContext: AppContext,
        fetchedEncryptedKeys: [KeyDetails],
        router: GlobalRouterType = GlobalRouter(),


Router not needed here, can get from context

tomholub · 2021-12-01T11:30:25Z

FlowCrypt/Controllers/Setup/SetupCreatePassphraseAbstractViewController.swift

-    let core: Core
+    let core: Core = Core.shared
    let router: GlobalRouterType


core and router not needed here in init

tomholub · 2021-12-01T11:30:43Z

FlowCrypt/Controllers/Setup/SetupEKMKeyViewController.swift

        user: UserId,
        keys: [KeyDetails] = [],
-        core: Core = .shared,
        router: GlobalRouterType = GlobalRouter(),


router not needed, get from ctx

tomholub · 2021-12-01T11:30:56Z

FlowCrypt/Controllers/Setup/SetupGenerateKeyViewController.swift

        user: UserId,
-        backupService: BackupServiceType = BackupService(),
-        core: Core = .shared,
        router: GlobalRouterType = GlobalRouter(),


can get router from ctx instead

tomholub · 2021-12-01T21:42:22Z

FlowCrypt/Functionality/Services/Folders Services/TrashFolderProvider.swift

+        // todo - rename argument to folderService:
+        user: User,
+        folderProvider: FoldersServiceType,


rename folderProvider to folderService here

tomholub · 2021-12-01T21:44:01Z

FlowCrypt/Functionality/Services/GlobalRouter.swift

-        logger.logInfo("proceed for session \(session.debugDescription)")
-        AppStartup().initializeApp(window: keyWindow, session: session)
+    private func proceed(with appContext: AppContext) {
+        logger.logInfo("proceed for session: \(appContext.session?.description ?? "nil")")


originally it used debugDescription, not sure of the difference

tomholub · 2021-12-01T21:45:13Z

FlowCrypt/Functionality/Services/Local Private Key Services/KeyDataStorage.swift

+// todo - what is this and what is it used for?
 final class KeyDataStorage {


Please remove this class, it just wraps functionality with no value add

tomholub · 2021-12-01T21:45:53Z

FlowCrypt/Functionality/Services/Local Private Key Services/KeyService.swift

-        storage: KeyStorageType = KeyDataStorage(),
-        passPhraseService: PassPhraseServiceType = PassPhraseService(),
-        currentUserEmail: @autoclosure @escaping () -> (String?) = DataService.shared.email
+        storage: KeyStorageType,


please rename storage to keyStorage here

FlowCrypt/Functionality/Services/Local Private Key Services/PassPhraseService.swift

tomholub added 11 commits November 30, 2021 00:08

issue 1111 only initialize Realm and KeyChainService once

cf00351

wip

35f2086

wip

0e2a495

wip

aca0f96

[skip ci] fixed controllers

f2d01f8

[skip ci] more fixes

353ff23

[skip ci] a few more

7302dbd

[skip ci] fix services

bd91385

mail provider fixes [skip ci]

f62f156

a few more fixes [skip ci]

70509db

a few more fixes [skip ci]

7e5accd

ivan-ushakov mentioned this pull request Nov 30, 2021

use throwing var methods for realm references to other objects #1035

Closed

it builds

bbf37a9

tomholub changed the title ~~[wip] issue 1111 only initialize Realm and KeyChainService once~~ issue 1111 only initialize Realm and KeyChainService once Nov 30, 2021

tomholub requested review from ivan-ushakov and sosnovsky November 30, 2021 18:04

tomholub commented Nov 30, 2021

View reviewed changes

tomholub added 2 commits November 30, 2021 20:09

add to test scope

08941db

Merge branch 'master' into issue-1111-realm-keychain-init

143f9b7

[skip ci] remove AppReset, fix some test usages

40738e5

ivan-ushakov marked this pull request as ready for review November 30, 2021 20:04

ivan-ushakov marked this pull request as draft November 30, 2021 20:04

ivan-ushakov reviewed Nov 30, 2021

View reviewed changes

tomholub mentioned this pull request Dec 1, 2021

no need custom main file #1130

Closed

tomholub mentioned this pull request Dec 1, 2021

look into in-memory Realm for tests #1131

Closed

issue #1111 fix unit tests running

5947f39

issue #1131 use in-memory Realm for tests

050d226

issue #1111 fix tests

359030b

sosnovsky self-requested a review December 1, 2021 15:07

sosnovsky approved these changes Dec 1, 2021

View reviewed changes

tomholub marked this pull request as ready for review December 1, 2021 15:11

tomholub merged commit 4162138 into master Dec 1, 2021

tomholub deleted the issue-1111-realm-keychain-init branch December 1, 2021 15:12

tomholub mentioned this pull request Dec 1, 2021

re-evaluate Realm usages - currently on main thread? #1134

Closed

tomholub commented Dec 1, 2021

View reviewed changes

This was referenced Dec 1, 2021

cleanups after Realm init AppContext PR #1135

Closed

confusing naming of password storage init properties #1136

Closed

		// todo - what is this and what is it used for?
		final class KeyDataStorage {

issue 1111 only initialize Realm and KeyChainService once #1117

issue 1111 only initialize Realm and KeyChainService once #1117

Conversation

tomholub commented Nov 29, 2021

To be filled by reviewers

tomholub commented Nov 30, 2021

tomholub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-ushakov commented Nov 30, 2021

tomholub commented Nov 30, 2021 • edited Loading

tomholub commented Nov 30, 2021

ivan-ushakov commented Nov 30, 2021

tomholub commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

ivan-ushakov commented Nov 30, 2021 • edited Loading

ivan-ushakov commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

tomholub commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

tomholub commented Dec 1, 2021

tomholub commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

tomholub commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

tomholub commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

sosnovsky commented Dec 1, 2021

tomholub commented Dec 1, 2021

tomholub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomholub commented Nov 30, 2021 •

edited

Loading

ivan-ushakov commented Nov 30, 2021 •

edited

Loading