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.

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

fix: avoid reward redemption crash via buffer refactor #4949

Merged
merged 15 commits into from
Nov 8, 2023

Conversation

iProdigy
Copy link
Contributor

@iProdigy iProdigy commented Nov 6, 2023

Closes #2432
Closes #3942

Supersedes #4942

Inspired by #4943 (review)

Rewrites self-destructing signal usage (that suffered from inadequate value forwarding & use-after-free) with buffer approach that holds redemptions without a corresponding reward until a relevant pubsub message arrives

Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

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

Works for me and is easier to debug. 👍

src/providers/twitch/TwitchChannel.hpp Outdated Show resolved Hide resolved
src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved
@@ -218,8 +227,7 @@ class TwitchChannel final : public Channel, public ChannelChatters
pajlada::Signals::NoArgSignal roomModesChanged;

// Channel point rewards
pajlada::Signals::SelfDisconnectingSignal<ChannelPointReward>
channelPointRewardAdded;
boost::circular_buffer_space_optimized<QueuedRedemption> waitingRedemptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use the aspect that this is a circular buffer? This seems like it could be a std::vector.

Copy link
Contributor Author

@iProdigy iProdigy Nov 6, 2023

Choose a reason for hiding this comment

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

This has the advantage of automatically dropping the oldest queued redemptions when many new redemptions are being queued

Imagine a redemption (for an unknown reward) occurs while pubsub is disconnected. Later, the reward is deleted and pubsub reconnects. With a vector, we'd never delete this redemption. With the buffer, we place a limit on how many outstanding redemptions can be held in memory. (this can be replaced with vector but erase(begin) is $O(n)$ for vector while circular buffer just overwrites the element in the first position in $O(1)$ time)

Copy link
Contributor Author

@iProdigy iProdigy Nov 6, 2023

Choose a reason for hiding this comment

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

Since deletions can happen in the middle of the collection, perhaps we should use std::list instead to avoid copying elements (and can easily implement max size logic ourselves)

Copy link
Contributor Author

@iProdigy iProdigy Nov 6, 2023

Choose a reason for hiding this comment

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

Switched to std::list in 5010223

In reality, most of the operations are (1) add element to empty collection and (2) remove singular element from collection (rendering it empty). This is $O(1)$ with std::list but we may want to consider whether std::vector has a small memory footprint here (if we prefer to optimize for the typical use case rather than edge cases)

Edit: when there are multiple elements in the collection, we have to iterate to determine which elements to remove. this is likely faster for vector and buffer due to contiguous memory allocation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved
src/providers/twitch/TwitchChannel.hpp Outdated Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

This looks good to me, and is easier to reason about 👍 Thank you for taking the time to dig into this!

@pajlada
Copy link
Member

pajlada commented Nov 8, 2023

@Nerixyz @iProdigy if either of you could take a peek at the comment I added in 1458b0b that would be good - once that's done I'll merge this in

@iProdigy
Copy link
Contributor Author

iProdigy commented Nov 8, 2023

Comment looks good to me!

@Nerixyz
Copy link
Contributor

Nerixyz commented Nov 8, 2023

The type name ChannelPointReward is a bit confusing. It suggests that we get the reward definition while we actually get a redemption that happens to include the reward definition. But that's for a future PR.

@pajlada pajlada merged commit d40b0a6 into Chatterino:master Nov 8, 2023
15 checks passed
@iProdigy iProdigy deleted the fix/redemption-crash branch November 8, 2023 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Channel Point crash Chatterino SIGABRT due to redemptions
3 participants