From 297e38b8b6f3cb7a3c8b018b1841c62ff1a806af Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 19 Oct 2022 15:14:55 -0700 Subject: [PATCH 1/6] Move to mutable lambda --- .../client_wrapper/event_channel_unittests.cc | 57 ++++++++++++++++--- .../include/flutter/event_channel.h | 19 +++---- 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/shell/platform/common/client_wrapper/event_channel_unittests.cc b/shell/platform/common/client_wrapper/event_channel_unittests.cc index 395913870f644..c2c3748b9b3dc 100644 --- a/shell/platform/common/client_wrapper/event_channel_unittests.cc +++ b/shell/platform/common/client_wrapper/event_channel_unittests.cc @@ -33,7 +33,7 @@ class TestBinaryMessenger : public BinaryMessenger { return last_message_handler_channel_; } - BinaryMessageHandler last_message_handler() { return last_message_handler_; } + BinaryMessageHandler& last_message_handler() { return last_message_handler_; } private: std::string last_message_handler_channel_; @@ -64,7 +64,7 @@ TEST(EventChannelTest, Registration) { EXPECT_EQ(messenger.last_message_handler_channel(), channel_name); EXPECT_NE(messenger.last_message_handler(), nullptr); - // Send dummy listen message. + // Send test listen message. MethodCall<> call("listen", nullptr); auto message = codec.EncodeMethodCall(call); messenger.last_message_handler()( @@ -121,7 +121,7 @@ TEST(EventChannelTest, Cancel) { EXPECT_EQ(messenger.last_message_handler_channel(), channel_name); EXPECT_NE(messenger.last_message_handler(), nullptr); - // Send dummy listen message. + // Send test listen message. MethodCall<> call_listen("listen", nullptr); auto message = codec.EncodeMethodCall(call_listen); messenger.last_message_handler()( @@ -129,7 +129,7 @@ TEST(EventChannelTest, Cancel) { [](const uint8_t* reply, const size_t reply_size) {}); EXPECT_EQ(on_listen_called, true); - // Send dummy cancel message. + // Send test cancel message. MethodCall<> call_cancel("cancel", nullptr); message = codec.EncodeMethodCall(call_cancel); messenger.last_message_handler()( @@ -165,7 +165,7 @@ TEST(EventChannelTest, ListenNotCancel) { EXPECT_EQ(messenger.last_message_handler_channel(), channel_name); EXPECT_NE(messenger.last_message_handler(), nullptr); - // Send dummy listen message. + // Send test listen message. MethodCall<> call_listen("listen", nullptr); auto message = codec.EncodeMethodCall(call_listen); messenger.last_message_handler()( @@ -205,7 +205,7 @@ TEST(EventChannelTest, ReRegistration) { EXPECT_EQ(messenger.last_message_handler_channel(), channel_name); EXPECT_NE(messenger.last_message_handler(), nullptr); - // Send dummy listen message. + // Send test listen message. MethodCall<> call("listen", nullptr); auto message = codec.EncodeMethodCall(call); messenger.last_message_handler()( @@ -213,7 +213,7 @@ TEST(EventChannelTest, ReRegistration) { [](const uint8_t* reply, const size_t reply_size) {}); EXPECT_EQ(on_listen_called, true); - // Send second dummy message to test StreamHandler's OnCancel + // Send second test message to test StreamHandler's OnCancel // method is called before OnListen method is called. on_listen_called = false; message = codec.EncodeMethodCall(call); @@ -226,4 +226,47 @@ TEST(EventChannelTest, ReRegistration) { EXPECT_EQ(on_listen_called, true); } +// Test that the handler's lifetime can exceed its event channel. +TEST(EventChannelTest, HandlerCanOutliveEventChannel) { + TestBinaryMessenger messenger; + const std::string channel_name("some_channel"); + const StandardMethodCodec& codec = StandardMethodCodec::GetInstance(); + + bool on_listen_called = false; + bool on_cancel_called = false; + { + EventChannel channel(&messenger, channel_name, &codec); + auto handler = std::make_unique>( + [&on_listen_called](const EncodableValue* arguments, + std::unique_ptr>&& events) + -> std::unique_ptr> { + on_listen_called = true; + return nullptr; + }, + [&on_cancel_called](const EncodableValue* arguments) + -> std::unique_ptr> { + on_cancel_called = true; + return nullptr; + }); + channel.SetStreamHandler(std::move(handler)); + } + + // The event channel is now destroyed. + // Send test listen message. + MethodCall<> call_listen("listen", nullptr); + auto message = codec.EncodeMethodCall(call_listen); + messenger.last_message_handler()( + message->data(), message->size(), + [](const uint8_t* reply, const size_t reply_size) {}); + EXPECT_EQ(on_listen_called, true); + + // Send test cancel message. + MethodCall<> call_cancel("cancel", nullptr); + message = codec.EncodeMethodCall(call_cancel); + messenger.last_message_handler()( + message->data(), message->size(), + [](const uint8_t* reply, const size_t reply_size) {}); + EXPECT_EQ(on_cancel_called, true); +} + } // namespace flutter diff --git a/shell/platform/common/client_wrapper/include/flutter/event_channel.h b/shell/platform/common/client_wrapper/include/flutter/event_channel.h index fe0e1414d63da..1afaf0ede836b 100644 --- a/shell/platform/common/client_wrapper/include/flutter/event_channel.h +++ b/shell/platform/common/client_wrapper/include/flutter/event_channel.h @@ -50,7 +50,6 @@ class EventChannel { void SetStreamHandler(std::unique_ptr> handler) { if (!handler) { messenger_->SetMessageHandler(name_, nullptr); - is_listening_ = false; return; } @@ -61,11 +60,12 @@ class EventChannel { const MethodCodec* codec = codec_; const std::string channel_name = name_; const BinaryMessenger* messenger = messenger_; + bool is_listening = false; BinaryMessageHandler binary_handler = [shared_handler, codec, channel_name, - messenger, - this](const uint8_t* message, - const size_t message_size, - BinaryReply reply) { + messenger, is_listening]( + const uint8_t* message, + const size_t message_size, + BinaryReply reply) mutable { constexpr char kOnListenMethod[] = "listen"; constexpr char kOnCancelMethod[] = "cancel"; @@ -80,7 +80,7 @@ class EventChannel { const std::string& method = method_call->method_name(); if (method.compare(kOnListenMethod) == 0) { - if (is_listening_) { + if (is_listening) { std::unique_ptr> error = shared_handler->OnCancel(nullptr); if (error) { @@ -89,7 +89,7 @@ class EventChannel { << ", " << (error->error_details); } } - is_listening_ = true; + is_listening = true; std::unique_ptr> result; auto sink = std::make_unique( @@ -105,7 +105,7 @@ class EventChannel { reply(result->data(), result->size()); } else if (method.compare(kOnCancelMethod) == 0) { std::unique_ptr> result; - if (is_listening_) { + if (is_listening) { std::unique_ptr> error = shared_handler->OnCancel(method_call->arguments()); if (error) { @@ -114,7 +114,7 @@ class EventChannel { } else { result = codec->EncodeSuccessEnvelope(); } - is_listening_ = false; + is_listening = false; } else { result = codec->EncodeErrorEnvelope( "error", "No active stream to cancel", nullptr); @@ -165,7 +165,6 @@ class EventChannel { BinaryMessenger* messenger_; const std::string name_; const MethodCodec* codec_; - bool is_listening_ = false; }; } // namespace flutter From 89ff9c397a785301cda65d600b26462dea4875eb Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 19 Oct 2022 15:18:37 -0700 Subject: [PATCH 2/6] Tweak --- .../platform/common/client_wrapper/event_channel_unittests.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/platform/common/client_wrapper/event_channel_unittests.cc b/shell/platform/common/client_wrapper/event_channel_unittests.cc index c2c3748b9b3dc..67de3b409073c 100644 --- a/shell/platform/common/client_wrapper/event_channel_unittests.cc +++ b/shell/platform/common/client_wrapper/event_channel_unittests.cc @@ -226,8 +226,8 @@ TEST(EventChannelTest, ReRegistration) { EXPECT_EQ(on_listen_called, true); } -// Test that the handler's lifetime can exceed its event channel. -TEST(EventChannelTest, HandlerCanOutliveEventChannel) { +// Test that the handler is called even if the event channel is destroyed. +TEST(EventChannelTest, HandlerOutlivesEventChannel) { TestBinaryMessenger messenger; const std::string channel_name("some_channel"); const StandardMethodCodec& codec = StandardMethodCodec::GetInstance(); From 59a1082a329e69374ab8a32694efa3a2b67a95f0 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 19 Oct 2022 16:16:52 -0700 Subject: [PATCH 3/6] Copy comment from MethodChannel --- .../common/client_wrapper/include/flutter/event_channel.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shell/platform/common/client_wrapper/include/flutter/event_channel.h b/shell/platform/common/client_wrapper/include/flutter/event_channel.h index 1afaf0ede836b..fcb587eb8f077 100644 --- a/shell/platform/common/client_wrapper/include/flutter/event_channel.h +++ b/shell/platform/common/client_wrapper/include/flutter/event_channel.h @@ -47,6 +47,10 @@ class EventChannel { // Registers a stream handler on this channel. // If no handler has been registered, any incoming stream setup requests will // be handled silently by providing an empty stream. + // + // Note that the EventChannel does not own the handler and will not + // unregister it on destruction. The caller is responsible for unregistering + // the handler if it should no longer be called. void SetStreamHandler(std::unique_ptr> handler) { if (!handler) { messenger_->SetMessageHandler(name_, nullptr); From 4bbf1ce7e0fff8cbd7b19d60845c7ab16a528419 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 19 Oct 2022 16:21:54 -0700 Subject: [PATCH 4/6] Tweak --- .../common/client_wrapper/event_channel_unittests.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/shell/platform/common/client_wrapper/event_channel_unittests.cc b/shell/platform/common/client_wrapper/event_channel_unittests.cc index 67de3b409073c..b12401f43b66a 100644 --- a/shell/platform/common/client_wrapper/event_channel_unittests.cc +++ b/shell/platform/common/client_wrapper/event_channel_unittests.cc @@ -251,7 +251,10 @@ TEST(EventChannelTest, HandlerOutlivesEventChannel) { channel.SetStreamHandler(std::move(handler)); } - // The event channel is now destroyed. + // The event channel was destroyed but the handler should still be alive. + EXPECT_EQ(messenger.last_message_handler_channel(), channel_name); + EXPECT_NE(messenger.last_message_handler(), nullptr); + // Send test listen message. MethodCall<> call_listen("listen", nullptr); auto message = codec.EncodeMethodCall(call_listen); From 7825256d5459ec962a90cb0340f3312d58287eb6 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 19 Oct 2022 17:10:37 -0700 Subject: [PATCH 5/6] Feedback --- .../common/client_wrapper/event_channel_unittests.cc | 4 +++- .../client_wrapper/include/flutter/event_channel.h | 12 ++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/shell/platform/common/client_wrapper/event_channel_unittests.cc b/shell/platform/common/client_wrapper/event_channel_unittests.cc index b12401f43b66a..58005fcffffce 100644 --- a/shell/platform/common/client_wrapper/event_channel_unittests.cc +++ b/shell/platform/common/client_wrapper/event_channel_unittests.cc @@ -33,7 +33,9 @@ class TestBinaryMessenger : public BinaryMessenger { return last_message_handler_channel_; } - BinaryMessageHandler& last_message_handler() { return last_message_handler_; } + const BinaryMessageHandler& last_message_handler() { + return last_message_handler_; + } private: std::string last_message_handler_channel_; diff --git a/shell/platform/common/client_wrapper/include/flutter/event_channel.h b/shell/platform/common/client_wrapper/include/flutter/event_channel.h index fcb587eb8f077..0b7c1ef821d97 100644 --- a/shell/platform/common/client_wrapper/include/flutter/event_channel.h +++ b/shell/platform/common/client_wrapper/include/flutter/event_channel.h @@ -64,12 +64,12 @@ class EventChannel { const MethodCodec* codec = codec_; const std::string channel_name = name_; const BinaryMessenger* messenger = messenger_; - bool is_listening = false; + std::shared_ptr is_listening = std::make_shared(false); BinaryMessageHandler binary_handler = [shared_handler, codec, channel_name, messenger, is_listening]( const uint8_t* message, const size_t message_size, - BinaryReply reply) mutable { + BinaryReply reply) { constexpr char kOnListenMethod[] = "listen"; constexpr char kOnCancelMethod[] = "cancel"; @@ -84,7 +84,7 @@ class EventChannel { const std::string& method = method_call->method_name(); if (method.compare(kOnListenMethod) == 0) { - if (is_listening) { + if (*is_listening) { std::unique_ptr> error = shared_handler->OnCancel(nullptr); if (error) { @@ -93,7 +93,7 @@ class EventChannel { << ", " << (error->error_details); } } - is_listening = true; + *is_listening = true; std::unique_ptr> result; auto sink = std::make_unique( @@ -109,7 +109,7 @@ class EventChannel { reply(result->data(), result->size()); } else if (method.compare(kOnCancelMethod) == 0) { std::unique_ptr> result; - if (is_listening) { + if (*is_listening) { std::unique_ptr> error = shared_handler->OnCancel(method_call->arguments()); if (error) { @@ -118,7 +118,7 @@ class EventChannel { } else { result = codec->EncodeSuccessEnvelope(); } - is_listening = false; + *is_listening = false; } else { result = codec->EncodeErrorEnvelope( "error", "No active stream to cancel", nullptr); From 435c7ec185fb4209389e50a3e007af427c50eed1 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Fri, 21 Oct 2022 11:29:45 -0700 Subject: [PATCH 6/6] Go back to mutable lambda approach --- .../include/flutter/event_channel.h | 127 +++++++++--------- 1 file changed, 66 insertions(+), 61 deletions(-) diff --git a/shell/platform/common/client_wrapper/include/flutter/event_channel.h b/shell/platform/common/client_wrapper/include/flutter/event_channel.h index 0b7c1ef821d97..08c242a2ff441 100644 --- a/shell/platform/common/client_wrapper/include/flutter/event_channel.h +++ b/shell/platform/common/client_wrapper/include/flutter/event_channel.h @@ -64,70 +64,75 @@ class EventChannel { const MethodCodec* codec = codec_; const std::string channel_name = name_; const BinaryMessenger* messenger = messenger_; - std::shared_ptr is_listening = std::make_shared(false); - BinaryMessageHandler binary_handler = [shared_handler, codec, channel_name, - messenger, is_listening]( - const uint8_t* message, - const size_t message_size, - BinaryReply reply) { - constexpr char kOnListenMethod[] = "listen"; - constexpr char kOnCancelMethod[] = "cancel"; - - std::unique_ptr> method_call = - codec->DecodeMethodCall(message, message_size); - if (!method_call) { - std::cerr << "Unable to construct method call from message on channel: " - << channel_name << std::endl; - reply(nullptr, 0); - return; - } - - const std::string& method = method_call->method_name(); - if (method.compare(kOnListenMethod) == 0) { - if (*is_listening) { - std::unique_ptr> error = - shared_handler->OnCancel(nullptr); - if (error) { - std::cerr << "Failed to cancel existing stream: " - << (error->error_code) << ", " << (error->error_message) - << ", " << (error->error_details); + BinaryMessageHandler binary_handler = + [shared_handler, codec, channel_name, messenger, + // Mutable state to track the handler's listening status. + is_listening = bool(false)](const uint8_t* message, + const size_t message_size, + BinaryReply reply) mutable { + constexpr char kOnListenMethod[] = "listen"; + constexpr char kOnCancelMethod[] = "cancel"; + + std::unique_ptr> method_call = + codec->DecodeMethodCall(message, message_size); + if (!method_call) { + std::cerr + << "Unable to construct method call from message on channel: " + << channel_name << std::endl; + reply(nullptr, 0); + return; } - } - *is_listening = true; - - std::unique_ptr> result; - auto sink = std::make_unique( - messenger, channel_name, codec); - std::unique_ptr> error = - shared_handler->OnListen(method_call->arguments(), std::move(sink)); - if (error) { - result = codec->EncodeErrorEnvelope( - error->error_code, error->error_message, error->error_details); - } else { - result = codec->EncodeSuccessEnvelope(); - } - reply(result->data(), result->size()); - } else if (method.compare(kOnCancelMethod) == 0) { - std::unique_ptr> result; - if (*is_listening) { - std::unique_ptr> error = - shared_handler->OnCancel(method_call->arguments()); - if (error) { - result = codec->EncodeErrorEnvelope( - error->error_code, error->error_message, error->error_details); + + const std::string& method = method_call->method_name(); + if (method.compare(kOnListenMethod) == 0) { + if (is_listening) { + std::unique_ptr> error = + shared_handler->OnCancel(nullptr); + if (error) { + std::cerr << "Failed to cancel existing stream: " + << (error->error_code) << ", " + << (error->error_message) << ", " + << (error->error_details); + } + } + is_listening = true; + + std::unique_ptr> result; + auto sink = std::make_unique( + messenger, channel_name, codec); + std::unique_ptr> error = + shared_handler->OnListen(method_call->arguments(), + std::move(sink)); + if (error) { + result = codec->EncodeErrorEnvelope(error->error_code, + error->error_message, + error->error_details); + } else { + result = codec->EncodeSuccessEnvelope(); + } + reply(result->data(), result->size()); + } else if (method.compare(kOnCancelMethod) == 0) { + std::unique_ptr> result; + if (is_listening) { + std::unique_ptr> error = + shared_handler->OnCancel(method_call->arguments()); + if (error) { + result = codec->EncodeErrorEnvelope(error->error_code, + error->error_message, + error->error_details); + } else { + result = codec->EncodeSuccessEnvelope(); + } + is_listening = false; + } else { + result = codec->EncodeErrorEnvelope( + "error", "No active stream to cancel", nullptr); + } + reply(result->data(), result->size()); } else { - result = codec->EncodeSuccessEnvelope(); + reply(nullptr, 0); } - *is_listening = false; - } else { - result = codec->EncodeErrorEnvelope( - "error", "No active stream to cancel", nullptr); - } - reply(result->data(), result->size()); - } else { - reply(nullptr, 0); - } - }; + }; messenger_->SetMessageHandler(name_, std::move(binary_handler)); }