Repository navigation
[pigeon] Allow event channel unregistration - #13438
auto-submit[bot] merged 8 commits into
Conversation
Makes the handler parameter to event channel registration nullable, and makes passing null/nil unregister the previously registered handler, allowing clients to unergister handlers to prevent leaks in way that is consistent with host API unregistration. Fixes flutter/flutter#195049
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for removing event channel handler registration in Kotlin and Swift by passing null to the stream handler registration method, updating the generators, generated code, and tests accordingly. The review feedback highlights that using static registeredChannels collections to track active channels in both Kotlin and Swift generators introduces severe memory leaks across FlutterEngine instances. The reviewer suggests directly unregistering the handlers via the binary messenger to avoid static state entirely.
| companion object { | ||
| fun register(messenger: BinaryMessenger, streamHandler: ${toUpperCamelCase(func.name)}StreamHandler, instanceName: String = "") { | ||
| private var registeredChannels = mutableMapOf<String, EventChannel>() | ||
|
|
||
| fun register(messenger: BinaryMessenger, streamHandler: ${toUpperCamelCase(func.name)}StreamHandler?, instanceName: String = "") { | ||
| var channelName: String = "${makeChannelName(api, func, dartPackageName)}" | ||
| if (instanceName.isNotEmpty()) { | ||
| channelName += ".\$instanceName" | ||
| } | ||
| val internalStreamHandler = ${generatorOptions.fileSpecificClassNameComponent}PigeonStreamHandler<${_kotlinTypeForDartType(func.returnType)}>(streamHandler) | ||
| EventChannel(messenger, channelName, ${generatorOptions.fileSpecificClassNameComponent}$_pigeonMethodCodec).setStreamHandler(internalStreamHandler) | ||
| val previousChannel = registeredChannels.remove(channelName) | ||
| if (previousChannel != null) { | ||
| previousChannel.setStreamHandler(null) | ||
| } | ||
| if (streamHandler != null) { | ||
| val internalStreamHandler = ${generatorOptions.fileSpecificClassNameComponent}PigeonStreamHandler<${_kotlinTypeForDartType(func.returnType)}>(streamHandler) | ||
| val channel = EventChannel(messenger, channelName, ${generatorOptions.fileSpecificClassNameComponent}$_pigeonMethodCodec) | ||
| channel.setStreamHandler(internalStreamHandler) | ||
| registeredChannels[channelName] = channel | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using a static registeredChannels map to keep track of active EventChannels introduces a severe memory leak. In Android, the JVM process (and thus static variables) can persist across multiple FlutterEngine instances (e.g., during activity recreation or in add-to-app scenarios). Because the static map retains references to EventChannel instances from previous engines, those engines and their associated resources will be leaked.
Instead of keeping a static map of channels, we can directly unregister the handler by calling messenger.setMessageHandler(channelName, null). This is because EventChannel.setStreamHandler(null) internally just calls messenger.setMessageHandler(name, null). This completely avoids the need for static state, is thread-safe, and prevents memory leaks.
| companion object { | |
| fun register(messenger: BinaryMessenger, streamHandler: ${toUpperCamelCase(func.name)}StreamHandler, instanceName: String = "") { | |
| private var registeredChannels = mutableMapOf<String, EventChannel>() | |
| fun register(messenger: BinaryMessenger, streamHandler: ${toUpperCamelCase(func.name)}StreamHandler?, instanceName: String = "") { | |
| var channelName: String = "${makeChannelName(api, func, dartPackageName)}" | |
| if (instanceName.isNotEmpty()) { | |
| channelName += ".\$instanceName" | |
| } | |
| val internalStreamHandler = ${generatorOptions.fileSpecificClassNameComponent}PigeonStreamHandler<${_kotlinTypeForDartType(func.returnType)}>(streamHandler) | |
| EventChannel(messenger, channelName, ${generatorOptions.fileSpecificClassNameComponent}$_pigeonMethodCodec).setStreamHandler(internalStreamHandler) | |
| val previousChannel = registeredChannels.remove(channelName) | |
| if (previousChannel != null) { | |
| previousChannel.setStreamHandler(null) | |
| } | |
| if (streamHandler != null) { | |
| val internalStreamHandler = ${generatorOptions.fileSpecificClassNameComponent}PigeonStreamHandler<${_kotlinTypeForDartType(func.returnType)}>(streamHandler) | |
| val channel = EventChannel(messenger, channelName, ${generatorOptions.fileSpecificClassNameComponent}$_pigeonMethodCodec) | |
| channel.setStreamHandler(internalStreamHandler) | |
| registeredChannels[channelName] = channel | |
| } | |
| } | |
| } | |
| companion object { | |
| fun register(messenger: BinaryMessenger, streamHandler: ${toUpperCamelCase(func.name)}StreamHandler?, instanceName: String = "") { | |
| var channelName: String = "${makeChannelName(api, func, dartPackageName)}" | |
| if (instanceName.isNotEmpty()) { | |
| channelName += ".\$instanceName" | |
| } | |
| if (streamHandler == null) { | |
| messenger.setMessageHandler(channelName, null) | |
| } else { | |
| val internalStreamHandler = ${generatorOptions.fileSpecificClassNameComponent}PigeonStreamHandler<${_kotlinTypeForDartType(func.returnType)}>(streamHandler) | |
| val channel = EventChannel(messenger, channelName, ${generatorOptions.fileSpecificClassNameComponent}$_pigeonMethodCodec) | |
| channel.setStreamHandler(internalStreamHandler) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
those engines and their associated resources will be leaked.
I'm skeptical that this leaks the entire engine (if it does, the Android engine should get a weak proxy object like iOS got), but will will potentially accumulate channels from plugins that don't do cleanup, which isn't ideal.
More importantly, I forgot to consider multi-engine cases in keying the dictionary, so registering the same plugin on multiple engines will actually stomp the connection.
I can fix the latter by changing the dictionary, but maybe it's better to return an opaque wrapper around the event channel instead, and let that be used for cleanup. It makes the unregistration API different, but it means that nothing can outlive the combination of engine+plugin since there won't be an external reference.
Instead of keeping a static map of channels, we can directly unregister the handler by calling
messenger.setMessageHandler(channelName, null). This is becauseEventChannel.setStreamHandler(null)internally just callsmessenger.setMessageHandler(name, null). This completely avoids the need for static state, is thread-safe, and prevents memory leaks.
That is currently how it works, but we shouldn't rely on it. Exhibit A, the iOS behavior, and my comment about that it the issue.
| class ${toUpperCamelCase(func.name)}StreamHandler: PigeonEventChannelWrapper<${_swiftTypeForDartType(func.returnType)}> { | ||
| private static var registeredChannels: [String: FlutterEventChannel] = [:] | ||
| static func register(with messenger: FlutterBinaryMessenger, | ||
| instanceName: String = "", | ||
| streamHandler: ${toUpperCamelCase(func.name)}StreamHandler) { | ||
| streamHandler: ${toUpperCamelCase(func.name)}StreamHandler?) { | ||
| var channelName = "${makeChannelName(api, func, dartPackageName)}" | ||
| if !instanceName.isEmpty { | ||
| channelName += ".\\(instanceName)" | ||
| } | ||
| let internalStreamHandler = PigeonStreamHandler<${_swiftTypeForDartType(func.returnType)}>(wrapper: streamHandler) | ||
| let channel = FlutterEventChannel(name: channelName, binaryMessenger: messenger, codec: ${_getMethodCodecVarName(generatorOptions)}) | ||
| channel.setStreamHandler(internalStreamHandler) | ||
| if let previousChannel = registeredChannels.removeValue(forKey: channelName) { | ||
| previousChannel.setStreamHandler(nil) | ||
| } | ||
| if let streamHandler = streamHandler { | ||
| let internalStreamHandler = PigeonStreamHandler<${_swiftTypeForDartType(func.returnType)}>(wrapper: streamHandler) | ||
| let channel = FlutterEventChannel(name: channelName, binaryMessenger: messenger, codec: ${_getMethodCodecVarName(generatorOptions)}) | ||
| channel.setStreamHandler(internalStreamHandler) | ||
| registeredChannels[channelName] = channel | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using a static registeredChannels dictionary to keep track of active FlutterEventChannels introduces a severe memory leak. In iOS, static variables persist as long as the app process is alive. If the FlutterEngine is destroyed and recreated, the static dictionary will retain references to FlutterEventChannel instances from the previous engine, leaking the entire engine and its resources.
Instead of keeping a static dictionary of channels, we can directly unregister the handler by calling messenger.setMessageHandlerOnChannel(channelName, binaryMessageHandler: nil). This is because FlutterEventChannel.setStreamHandler(nil) internally just calls [_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:nil]. This completely avoids the need for static state, is thread-safe, and prevents memory leaks.
| class ${toUpperCamelCase(func.name)}StreamHandler: PigeonEventChannelWrapper<${_swiftTypeForDartType(func.returnType)}> { | |
| private static var registeredChannels: [String: FlutterEventChannel] = [:] | |
| static func register(with messenger: FlutterBinaryMessenger, | |
| instanceName: String = "", | |
| streamHandler: ${toUpperCamelCase(func.name)}StreamHandler) { | |
| streamHandler: ${toUpperCamelCase(func.name)}StreamHandler?) { | |
| var channelName = "${makeChannelName(api, func, dartPackageName)}" | |
| if !instanceName.isEmpty { | |
| channelName += ".\\(instanceName)" | |
| } | |
| let internalStreamHandler = PigeonStreamHandler<${_swiftTypeForDartType(func.returnType)}>(wrapper: streamHandler) | |
| let channel = FlutterEventChannel(name: channelName, binaryMessenger: messenger, codec: ${_getMethodCodecVarName(generatorOptions)}) | |
| channel.setStreamHandler(internalStreamHandler) | |
| if let previousChannel = registeredChannels.removeValue(forKey: channelName) { | |
| previousChannel.setStreamHandler(nil) | |
| } | |
| if let streamHandler = streamHandler { | |
| let internalStreamHandler = PigeonStreamHandler<${_swiftTypeForDartType(func.returnType)}>(wrapper: streamHandler) | |
| let channel = FlutterEventChannel(name: channelName, binaryMessenger: messenger, codec: ${_getMethodCodecVarName(generatorOptions)}) | |
| channel.setStreamHandler(internalStreamHandler) | |
| registeredChannels[channelName] = channel | |
| } | |
| } | |
| } | |
| class ${toUpperCamelCase(func.name)}StreamHandler: PigeonEventChannelWrapper<${_swiftTypeForDartType(func.returnType)}> { | |
| static func register(with messenger: FlutterBinaryMessenger, | |
| instanceName: String = "", | |
| streamHandler: ${toUpperCamelCase(func.name)}StreamHandler?) { | |
| var channelName = "${makeChannelName(api, func, dartPackageName)}" | |
| if !instanceName.isEmpty { | |
| channelName += ".\\(instanceName)" | |
| } | |
| if streamHandler == nil { | |
| messenger.setMessageHandlerOnChannel(channelName, binaryMessageHandler: nil) | |
| } else { | |
| let internalStreamHandler = PigeonStreamHandler<${_swiftTypeForDartType(func.returnType)}>(wrapper: streamHandler) | |
| let channel = FlutterEventChannel(name: channelName, binaryMessenger: messenger, codec: ${_getMethodCodecVarName(generatorOptions)}) | |
| channel.setStreamHandler(internalStreamHandler) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
This is because
FlutterEventChannel.setStreamHandler(nil)internally just calls[_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:nil].
This is a lie, which is why the PR didn't do that.
There was a problem hiding this comment.
Code Review
This pull request updates Pigeon to support removing event channel handler registrations in Kotlin and Swift by passing null to the stream handler registration method. It introduces new event channel wrapper classes in both Kotlin and Swift generators, and updates the register method to return these channel instances. Redundant return statements and open modifiers are also cleaned up in the Kotlin generator. The review feedback suggests adding the @discardableResult attribute to the generated Swift register method to prevent compiler warnings when callers ignore the returned channel instance.
| static func register( | ||
| with messenger: FlutterBinaryMessenger, | ||
| instanceName: String = "", | ||
| streamHandler: $streamHandlerClassName | ||
| ) -> $channelClassName { | ||
| let channel = $channelClassName(messenger: messenger, instanceName: instanceName) | ||
| channel.setStreamHandler(streamHandler) | ||
| return channel | ||
| } |
There was a problem hiding this comment.
In Swift, if a function returns a value and the caller does not use it, the compiler will emit an unused result warning unless the function is annotated with @discardableResult. Since the register method's return value can be safely ignored (as documented in its doc comment), we should add @discardableResult to prevent compiler warnings in existing codebases that upgrade to this version of Pigeon without capturing the returned channel.
| static func register( | |
| with messenger: FlutterBinaryMessenger, | |
| instanceName: String = "", | |
| streamHandler: $streamHandlerClassName | |
| ) -> $channelClassName { | |
| let channel = $channelClassName(messenger: messenger, instanceName: instanceName) | |
| channel.setStreamHandler(streamHandler) | |
| return channel | |
| } | |
| @discardableResult | |
| static func register( | |
| with messenger: FlutterBinaryMessenger, | |
| instanceName: String = "", | |
| streamHandler: $streamHandlerClassName | |
| ) -> $channelClassName { | |
| let channel = $channelClassName(messenger: messenger, instanceName: instanceName) | |
| channel.setStreamHandler(streamHandler) | |
| return channel | |
| } |
|
Design notes:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for removing event channel handler registration by passing null to the stream handler registration method in Pigeon. It updates the Kotlin and Swift generators to create a typed channel class for event streams, allowing for more flexible handler management. Additionally, it includes updates to the test plugins to exercise the new handler removal functionality. I have provided feedback regarding the use of @discardableResult in Swift to maintain backward compatibility and improve code cleanliness in the test suite.
| indent.addScoped('{', '}', () { | ||
| if (customType.type == CustomTypes.customClass) { | ||
| indent.write('return (readValue(buffer) as? List<Any?>)?.let '); | ||
| indent.write('(readValue(buffer) as? List<Any?>)?.let '); |
There was a problem hiding this comment.
This and the same change below are opportunistic cleanup of warnings I noticed in the IDE while prototyping solutions.
| } | ||
|
|
||
| interface ${generatorOptions.fileSpecificClassNameComponent}PigeonEventChannelWrapper<T> { | ||
| open fun onListen(p0: Any?, sink: PigeonEventSink<T>) {} |
There was a problem hiding this comment.
Same with these (open is the default for interfaces).
tarrinneal
left a comment
tarrinneal
left a comment
There was a problem hiding this comment.
Not sure why I didn't do this when I put this all together.
| * [kotlin,swift] Adds support for removing event channel handler registration | ||
| by passing `null` to the stream handler registration method. |
There was a problem hiding this comment.
Nit: It looks like this wording is from the earlier iteration where register accepted null—consider updating it to mention the new *EventChannel class and setStreamHandler(null).
There was a problem hiding this comment.
Oops, I rewrote the doc comments but forgot about the changelog. Fixed.
| // Store one handler to exercise the handler removal codepath, and discard | ||
| // the other to test that the return value can be safely ignored. | ||
| let streamHandlerOne = SendConsistentNumbers(numberToSend: 1) | ||
| let channelOne = StreamConsistentNumbersStreamHandler.register( | ||
| with: binaryMessenger, | ||
| instanceName: "1", | ||
| streamHandler: streamHandlerOne) | ||
| streamHandlerOne.onCompleted = { channelOne.setStreamHandler(nil) } | ||
| StreamConsistentNumbersStreamHandler.register( | ||
| with: binaryMessenger, instanceName: "1", | ||
| streamHandler: SendConsistentNumbers(numberToSend: 1)) | ||
| StreamConsistentNumbersStreamHandler.register( | ||
| with: binaryMessenger, instanceName: "2", | ||
| with: binaryMessenger, | ||
| instanceName: "2", | ||
| streamHandler: SendConsistentNumbers(numberToSend: 2)) |
There was a problem hiding this comment.
In TestPlugin.swift, channelOne.setStreamHandler(nil) runs in onCompleted after the 10th event, whereas TestPlugin.kt only calls eventChannelOne?.setStreamHandler(null) in onDetachedFromEngine, and integration_tests.dart finishes after the first stream completes. Would it be worth also unregistering on completion in TestPlugin.kt and asserting in integration_tests.dart that listening to instance "1" a second time fails (while instance "2" still succeeds)?
There was a problem hiding this comment.
I've moved the unregistration to match Swift. I wrote the Kotlin version first, but there's no corresponding detach call on the iOS side in the plugin currently which is why it was different in the first place. It's easy enough to have Kotlin match Swift so that the tests are easier to understand as a whole.
I don't think re-listening on the Dart side would fail; IIRC listening to a channel without a native handler just gives an empty stream. This is such a trivial passthrough now that I don't think we need to specifically test the behavior, since it would be very hard to regress without breaking compilation.
stuartmorgan
added
the
autosubmit
Adds a new class for each event channel API that wraps the underlying EventChannel and allows setting the stream handler, matching the underlying Flutter API. This allows clients that need to set a new handler later (notably, removing a handler to avoid an engine-lifetime reference to a handler for a transient channel) to do so, using the same API that non-Pigeon event channel code would use.
The existing static
registermethod is kept to avoid unnecessary breaking changes, but clients no longer need to use it if they want to create the channel directly.Fixes flutter/flutter#195049
Pre-Review Checklist
[shared_preferences]///).Footnotes
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