-
-
Notifications
You must be signed in to change notification settings - Fork 462
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
Conversation
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.
Works for me and is easier to debug. 👍
@@ -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; |
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.
Do we use the aspect that this is a circular buffer? This seems like it could be a std::vector
.
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 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
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.
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)
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.
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 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
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.
actually buffer may be best (i overestimated copying cost): https://www.codeproject.com/Articles/1185449/Performance-of-a-Circular-Buffer-vs-Vector-Deque-a / https://www.codeproject.com/Articles/340797/Number-crunching-Why-you-should-never-ever-EVER-us
Co-authored-by: nerix <nero.9@hotmail.de>
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 looks good to me, and is easier to reason about 👍 Thank you for taking the time to dig into this!
Comment looks good to me! |
The type name |
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