Skip to content

[google_maps_flutter] Adopts new async/await Swift Pigeon support - #12860

Open
stuartmorgan-g wants to merge 3 commits into
flutter:mainfrom
stuartmorgan-g:maps-swift-async-pigeon
Open

[google_maps_flutter] Adopts new async/await Swift Pigeon support#12860
stuartmorgan-g wants to merge 3 commits into
flutter:mainfrom
stuartmorgan-g:maps-swift-async-pigeon

Conversation

@stuartmorgan-g

Copy link
Copy Markdown
Collaborator

Switches from the callback-based Pigeon API to the new async/await-based API, which is:

  • more idiomatic modern Swift
  • safer, both since it uses Pigeon-generated actor annotations to ensure proper threading, and because it prevents the error case of calling the callback some number of times other than exactly once (which violates the engine API contract)
  • the syntax used by the new FFI backend that we plan to switch to in a follow-up

I verified that with this version of Pigeon we are no longer seeing thread violation logging in manual testing (see flutter/flutter#192199)

There is some extra Task boilerplate in many callbacks still because we are implementing APIs that are defined by the Google Maps API, and those are not yet using async and actor annotations, so we have to bridge them in the plugin layer.

Part of flutter/flutter#192720

Pre-Review Checklist

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the google_maps_flutter iOS packages (sdk9, sdk10, and shared) to adopt the new Pigeon async Swift support, converting callback-based asynchronous methods to Swift's async/throws syntax. Feedback on the changes identifies potential memory retention issues in TileOverlayController.swift across all three packages due to capturing self strongly inside the asynchronous Task closures, and recommends capturing self weakly to prevent delayed deallocation.

Comment on lines 108 to 135
override func requestTileFor(x: UInt, y: UInt, zoom: UInt, receiver: any GMSTileReceiver) {
DispatchQueue.main.async { [weak self] in
guard let self = self, let tileProviderDelegate = self.tileProviderDelegate else {
Task { @MainActor in
guard let tileProviderDelegate = self.tileProviderDelegate else {
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
return
}
tileProviderDelegate.tile(
withOverlayIdentifier: self.tileOverlayIdentifier,
location: PlatformPoint(x: Double(x), y: Double(y)),
zoom: Int64(zoom)
) { [weak self] result in
guard let self = self else {
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
return
var tileImage = kGMSTileLayerNoTile
do {
let tile = try await tileProviderDelegate.tile(
withOverlayIdentifier: self.tileOverlayIdentifier,
location: PlatformPoint(x: Double(x), y: Double(y)),
zoom: Int64(zoom)
)
if let data = tile.data?.data {
tileImage = self.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
}
var tileImage = kGMSTileLayerNoTile
switch result {
case .success(let tile):
if let data = tile.data?.data {
tileImage = self.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
}
case .failure(let error):
NSLog(
} catch {
let message =
if let error = error as? PigeonError {
"Can't get tile: errorCode = \(error.code), errorMessage = \(error.message ?? "nil"), details = \(error.details ?? "nil")"
)
}
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
} else {
"Can't get tile: unexpected error \(error)"
}
NSLog(message)
}
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Capturing self strongly inside the Task closure can lead to memory retention issues. Since tileProviderDelegate.tile is an asynchronous call (await), self (the TileProviderController layer) will be kept in memory for the entire duration of the platform channel call. If the layer is removed or the map is disposed while tile requests are in flight, this can delay deallocation of the layer and associated resources.

To prevent this, capture self weakly and retrieve the necessary properties before the asynchronous call, using optional chaining for the remaining references.

  override func requestTileFor(x: UInt, y: UInt, zoom: UInt, receiver: any GMSTileReceiver) {
    Task { [weak self] @MainActor in
      guard let tileProviderDelegate = self?.tileProviderDelegate,
            let tileOverlayIdentifier = self?.tileOverlayIdentifier else {
        receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
        return
      }
      var tileImage = kGMSTileLayerNoTile
      do {
        let tile = try await tileProviderDelegate.tile(
          withOverlayIdentifier: tileOverlayIdentifier,
          location: PlatformPoint(x: Double(x), y: Double(y)),
          zoom: Int64(zoom)
        )
        if let data = tile.data?.data {
          tileImage = self?.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
        }
      } catch {
        let message =
          if let error = error as? PigeonError {
            "Can't get tile: errorCode = \(error.code), errorMessage = \(error.message ?? "nil"), details = \(error.details ?? "nil")"
          } else {
            "Can't get tile: unexpected error \(error)"
          }
        NSLog(message)
      }
      receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
    }
  }

Comment on lines 108 to 135
override func requestTileFor(x: UInt, y: UInt, zoom: UInt, receiver: any GMSTileReceiver) {
DispatchQueue.main.async { [weak self] in
guard let self = self, let tileProviderDelegate = self.tileProviderDelegate else {
Task { @MainActor in
guard let tileProviderDelegate = self.tileProviderDelegate else {
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
return
}
tileProviderDelegate.tile(
withOverlayIdentifier: self.tileOverlayIdentifier,
location: PlatformPoint(x: Double(x), y: Double(y)),
zoom: Int64(zoom)
) { [weak self] result in
guard let self = self else {
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
return
var tileImage = kGMSTileLayerNoTile
do {
let tile = try await tileProviderDelegate.tile(
withOverlayIdentifier: self.tileOverlayIdentifier,
location: PlatformPoint(x: Double(x), y: Double(y)),
zoom: Int64(zoom)
)
if let data = tile.data?.data {
tileImage = self.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
}
var tileImage = kGMSTileLayerNoTile
switch result {
case .success(let tile):
if let data = tile.data?.data {
tileImage = self.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
}
case .failure(let error):
NSLog(
} catch {
let message =
if let error = error as? PigeonError {
"Can't get tile: errorCode = \(error.code), errorMessage = \(error.message ?? "nil"), details = \(error.details ?? "nil")"
)
}
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
} else {
"Can't get tile: unexpected error \(error)"
}
NSLog(message)
}
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Capturing self strongly inside the Task closure can lead to memory retention issues. Since tileProviderDelegate.tile is an asynchronous call (await), self (the TileProviderController layer) will be kept in memory for the entire duration of the platform channel call. If the layer is removed or the map is disposed while tile requests are in flight, this can delay deallocation of the layer and associated resources.

To prevent this, capture self weakly and retrieve the necessary properties before the asynchronous call, using optional chaining for the remaining references.

  override func requestTileFor(x: UInt, y: UInt, zoom: UInt, receiver: any GMSTileReceiver) {
    Task { [weak self] @MainActor in
      guard let tileProviderDelegate = self?.tileProviderDelegate,
            let tileOverlayIdentifier = self?.tileOverlayIdentifier else {
        receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
        return
      }
      var tileImage = kGMSTileLayerNoTile
      do {
        let tile = try await tileProviderDelegate.tile(
          withOverlayIdentifier: tileOverlayIdentifier,
          location: PlatformPoint(x: Double(x), y: Double(y)),
          zoom: Int64(zoom)
        )
        if let data = tile.data?.data {
          tileImage = self?.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
        }
      } catch {
        let message =
          if let error = error as? PigeonError {
            "Can't get tile: errorCode = \(error.code), errorMessage = \(error.message ?? "nil"), details = \(error.details ?? "nil")"
          } else {
            "Can't get tile: unexpected error \(error)"
          }
        NSLog(message)
      }
      receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
    }
  }

Comment on lines 108 to 135
override func requestTileFor(x: UInt, y: UInt, zoom: UInt, receiver: any GMSTileReceiver) {
DispatchQueue.main.async { [weak self] in
guard let self = self, let tileProviderDelegate = self.tileProviderDelegate else {
Task { @MainActor in
guard let tileProviderDelegate = self.tileProviderDelegate else {
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
return
}
tileProviderDelegate.tile(
withOverlayIdentifier: self.tileOverlayIdentifier,
location: PlatformPoint(x: Double(x), y: Double(y)),
zoom: Int64(zoom)
) { [weak self] result in
guard let self = self else {
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
return
var tileImage = kGMSTileLayerNoTile
do {
let tile = try await tileProviderDelegate.tile(
withOverlayIdentifier: self.tileOverlayIdentifier,
location: PlatformPoint(x: Double(x), y: Double(y)),
zoom: Int64(zoom)
)
if let data = tile.data?.data {
tileImage = self.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
}
var tileImage = kGMSTileLayerNoTile
switch result {
case .success(let tile):
if let data = tile.data?.data {
tileImage = self.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
}
case .failure(let error):
NSLog(
} catch {
let message =
if let error = error as? PigeonError {
"Can't get tile: errorCode = \(error.code), errorMessage = \(error.message ?? "nil"), details = \(error.details ?? "nil")"
)
}
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
} else {
"Can't get tile: unexpected error \(error)"
}
NSLog(message)
}
receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Capturing self strongly inside the Task closure can lead to memory retention issues. Since tileProviderDelegate.tile is an asynchronous call (await), self (the TileProviderController layer) will be kept in memory for the entire duration of the platform channel call. If the layer is removed or the map is disposed while tile requests are in flight, this can delay deallocation of the layer and associated resources.

To prevent this, capture self weakly and retrieve the necessary properties before the asynchronous call, using optional chaining for the remaining references.

  override func requestTileFor(x: UInt, y: UInt, zoom: UInt, receiver: any GMSTileReceiver) {
    Task { [weak self] @MainActor in
      guard let tileProviderDelegate = self?.tileProviderDelegate,
            let tileOverlayIdentifier = self?.tileOverlayIdentifier else {
        receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: kGMSTileLayerNoTile)
        return
      }
      var tileImage = kGMSTileLayerNoTile
      do {
        let tile = try await tileProviderDelegate.tile(
          withOverlayIdentifier: tileOverlayIdentifier,
          location: PlatformPoint(x: Double(x), y: Double(y)),
          zoom: Int64(zoom)
        )
        if let data = tile.data?.data {
          tileImage = self?.handleResultTile(UIImage(data: data)) ?? kGMSTileLayerNoTile
        }
      } catch {
        let message =
          if let error = error as? PigeonError {
            "Can't get tile: errorCode = \(error.code), errorMessage = \(error.message ?? "nil"), details = \(error.details ?? "nil")"
          } else {
            "Can't get tile: unexpected error \(error)"
          }
        NSLog(message)
      }
      receiver.receiveTileWith(x: x, y: y, zoom: zoom, image: tileImage)
    }
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD p: google_maps_flutter platform-ios triage-ios Should be looked at in iOS triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant