-
Notifications
You must be signed in to change notification settings - Fork 6k
[fuchsia] Log unregistered platform channels only once #18064
Conversation
| FML_LOG(ERROR) | ||
| << "Platform view received message on channel '" << message->channel() | ||
| << "' with no registered handler. And empty response will be " | ||
| "generated. Please implement the native message handler."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just mention that this message will only be logged once.
This is to reduce log spam. Fixes: flutter/flutter#55966
| << "Platform view received message on channel '" << message->channel() | ||
| << "' with no registered handler. And empty response will be " | ||
| "generated. Please implement the native message handler."; | ||
| const bool already_errored = unregistered_channels_.count(channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this additional check? if we're not in unregistered_channels_ then we've never logged before. This currently will log if we get a message on e.g. channelA, then channelB, channelC, etc won't log (as count() will be > 0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is count(channel), as in a count to check if the channel is in the set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I misread. sorry!
| // These are the channels that aren't registered and have been notified as | ||
| // such. Notifying via logs multiple times results in log-spam. See: | ||
| // https://github.com/flutter/flutter/issues/55966 | ||
| std::set<std::string /* channel */> unregistered_channels_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have multiple PlatformViews over the lifetime of a process? do we want this to be a static?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one platform view per engine so I think that should be fine. Given that what we are seeing now, it this message being printed at error levels every time we receive a platform message on the same channel, I changed it to be INFO and made it so that we log a channel once per engine. So I think this should reduce the problem by a lot. If we notice that we are still seeing spammy behavior making this a static is something we could consider.
This is to reduce log spam.
Fixes: flutter/flutter#55966