-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Tx subscription cleanup #1422
Tx subscription cleanup #1422
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,8 +51,12 @@ use fuel_core_types::{ | |
tai64::Tai64, | ||
}; | ||
|
||
use anyhow::anyhow; | ||
use parking_lot::Mutex as ParkingMutex; | ||
use std::sync::Arc; | ||
use std::{ | ||
sync::Arc, | ||
time::Duration, | ||
}; | ||
use tokio::{ | ||
sync::broadcast, | ||
time::MissedTickBehavior, | ||
|
@@ -76,9 +80,9 @@ pub struct TxStatusChange { | |
} | ||
|
||
impl TxStatusChange { | ||
pub fn new(capacity: usize) -> Self { | ||
pub fn new(capacity: usize, ttl: Duration) -> Self { | ||
let (new_tx_notification_sender, _) = broadcast::channel(capacity); | ||
let update_sender = UpdateSender::new(capacity); | ||
let update_sender = UpdateSender::new(capacity, ttl); | ||
Self { | ||
new_tx_notification_sender, | ||
update_sender, | ||
|
@@ -326,11 +330,11 @@ where | |
self.tx_status_sender.new_tx_notification_sender.subscribe() | ||
} | ||
|
||
pub async fn tx_update_subscribe(&self, tx_id: Bytes32) -> TxStatusStream { | ||
pub fn tx_update_subscribe(&self, tx_id: Bytes32) -> anyhow::Result<TxStatusStream> { | ||
self.tx_status_sender | ||
.update_sender | ||
.subscribe::<MpscChannel>(tx_id) | ||
.await | ||
.try_subscribe::<MpscChannel>(tx_id) | ||
.ok_or(anyhow!("Maximum number of subscriptions reached")) | ||
} | ||
} | ||
|
||
|
@@ -450,7 +454,14 @@ where | |
gossiped_tx_stream, | ||
committed_block_stream, | ||
shared: SharedState { | ||
tx_status_sender: TxStatusChange::new(number_of_active_subscription), | ||
tx_status_sender: TxStatusChange::new( | ||
number_of_active_subscription, | ||
// The connection should be closed automatically after the `SqueezedOut` event. | ||
// But because of slow/malicious consumers, the subscriber can still be occupied. | ||
// We allow the subscriber to receive the event produced by TxPool's TTL. | ||
// But we still want to drop subscribers after `2 * TxPool_TTL`. | ||
Comment on lines
+460
to
+462
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, the idea is to keep the subscription live long enough for one of:
Doubling the Txpool TTL allows us to use the second half of the doubled TTL for reporting the event. Is that more or less accurate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also gives a slow consumer enough time to read the event within some reasonable deadline. |
||
2 * config.transaction_ttl, | ||
), | ||
txpool, | ||
p2p, | ||
consensus_params, | ||
|
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.
It looks like this means 30 minutes? It seems long for a timeout - I would think closer to 30 seconds maybe?
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.
30s is too short, as it may take longer than that for transactions to make it into a block and users will have a subscription open during that whole duration. I agree this could be shorter, but this is a pretty safe default that will address some of the outstanding issues.