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.

Sign up for GitHub

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

Refactor FXIOS-10924 - Remove Deferred from FolderHierarchyFetcher #23900

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor FXIOS-10924 - Remove Deferred from FolderHierarchyFetcher #23900

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40579bf
Define protocol in rust places and implement its methods and use them…
PARAIPAN9 Dec 19, 2024
b7e1614
Remove RustPlacesProtocol
PARAIPAN9 Dec 20, 2024
7a4041b
Use defer for resuming continuation
PARAIPAN9 Dec 20, 2024
690f725
Update BookmarksHandlerMock class
PARAIPAN9 Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Utitlity/FolderHierarchyFetcher.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,23 @@ struct DefaultFolderHierarchyFetcher: FolderHierarchyFetcher, BookmarksRefactorF

func fetchFolders() async -> [Folder] {
let numDesktopBookmarks = await countDesktopBookmarks()
return await withCheckedContinuation { continuation in
profile.places.getBookmarksTree(rootGUID: rootFolderGUID,
recursive: true).uponQueue(.main) { data in
var folders = [Folder]()
defer {
continuation.resume(returning: folders)
}
guard let rootFolder = data.successValue as? BookmarkFolderData else { return }
let hasDesktopBookmarks = (numDesktopBookmarks ?? 0) > 0

let childrenFolders = rootFolder.children?.compactMap {
return $0 as? BookmarkFolderData
}
return await withUnsafeContinuation { continuation in
profile.places.getBookmarksTree(rootGUID: rootFolderGUID, recursive: true) { result in
switch result {
case .success(let data):
guard let rootFolder = data as? BookmarkFolderData else { return }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @PARAIPAN9 ! @lmarceau just wanted me to take a peek at the continuations.

There's a chance we miss calling continuation.resume here in the guard return since we don't have defer anymore. Are we explicitly removing defers as well as the Deferred?

let hasDesktopBookmarks = (numDesktopBookmarks ?? 0) > 0

for folder in childrenFolders ?? [] {
recursiveAddSubFolders(folder, folders: &folders, hasDesktopBookmarks: hasDesktopBookmarks)
let childrenFolders = rootFolder.children?.compactMap {
return $0 as? BookmarkFolderData
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also don't call the continuation here now either

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We call resume on both Result cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see, on the guard statement you're saying.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops yeah you're right that this one is in a compactMap, but yeah the guard one is a problem!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will put defer as it was before then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or just call the continuation in the guard would be reasonable, whichever you prefer!

If you use a defer, can you please check that the continuation doesn't get called twice with a breakpoint if possible? Since the unsafe version won't tell us if we're misusing the continuation, so we should be careful. 🙏 I'm worried the continuation resuming with folders below will also be triggered in the good case, as well as the defer, calling resume twice...

}
var folders = [Folder]()
for folder in childrenFolders ?? [] {
recursiveAddSubFolders(folder, folders: &folders, hasDesktopBookmarks: hasDesktopBookmarks)
}
continuation.resume(returning: folders)
case .failure:
continuation.resume(returning: [])
}
}
}
Expand All @@ -75,7 +76,7 @@ struct DefaultFolderHierarchyFetcher: FolderHierarchyFetcher, BookmarksRefactorF
}

private func countDesktopBookmarks() async -> Int? {
return await withCheckedContinuation { continuation in
return await withUnsafeContinuation { continuation in
profile.places.countBookmarksInTrees(folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) { result in
switch result {
case .success(let count):
Expand Down
56 changes: 56 additions & 0 deletions firefox-ios/Storage/Rust/RustPlaces.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ import struct MozillaAppServices.VisitTransitionSet

public protocol BookmarksHandler {
func getRecentBookmarks(limit: UInt, completion: @escaping ([BookmarkItemData]) -> Void)
func getBookmarksTree(
rootGUID: GUID,
recursive: Bool,
completion: @escaping (Result<BookmarkNodeData?, any Error>) -> Void
)
func getBookmarksTree(rootGUID: GUID, recursive: Bool) -> Deferred<Maybe<BookmarkNodeData?>>
func countBookmarksInTrees(folderGuids: [GUID], completion: @escaping (Result<Int, Error>) -> Void)
func updateBookmarkNode(
Expand Down Expand Up @@ -173,6 +178,42 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
return deferred
}

/// This method is reimplemented with a completion handler because we want to incrementally get rid of using `Deferred`.
public func withReader<T>(
_ callback: @escaping (
PlacesReadConnection
) throws -> T,
completion: @escaping (Result<T, any Error>) -> Void
) {
readerQueue.async {
guard self.isOpen else {
completion(.failure(PlacesConnectionError.connUseAfterApiClosed))
return
}

if self.reader == nil {
do {
self.reader = try self.api?.openReader()
} catch let error {
completion(.failure(error))
return
}
}

if let reader = self.reader {
do {
let result = try callback(reader)
completion(.success(result))
} catch let error {
completion(.failure(error))
return
}
} else {
completion(.failure(PlacesConnectionError.connUseAfterApiClosed))
}
}
}

public func getBookmarksTree(
rootGUID: GUID,
recursive: Bool
Expand All @@ -182,6 +223,21 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
}
}

public func getBookmarksTree(
rootGUID: GUID,
recursive: Bool,
completion: @escaping (Result<BookmarkNodeData?, any Error>) -> Void
) {
withReader(
{ connection in
return try connection.getBookmarksTree(
rootGUID: rootGUID,
recursive: recursive
)
},
completion: completion)
}

public func getBookmark(guid: GUID) -> Deferred<Maybe<BookmarkNodeData?>> {
return withReader { connection in
return try connection.getBookmark(guid: guid)
Expand Down