write proxy: expire old write delegation streams #723
Conversation
sqld/src/rpc/proxy.rs
Outdated
| let limit = std::time::Duration::from_secs(30); | ||
| for (client_id, db) in clients.iter() { | ||
| if db.idle_time() > limit { | ||
| to_remove.push(*client_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: this is a usecase for https://doc.rust-lang.org/stable/std/collections/struct.HashMap.html#method.retain
sqld/src/connection/mod.rs
Outdated
| impl<DB> TrackedConnection<DB> { | ||
| pub fn idle_time(&self) -> Duration { | ||
| let now = now_millis(); | ||
| let atime = self.atime.load(Ordering::Relaxed); | ||
| Duration::from_millis(now - atime) | ||
| } |
There was a problem hiding this comment.
I'm not sure this is safe. SystemTime is not monotonic, so there is a chance that now is in the past compared to time, and now - atime may overflow
There was a problem hiding this comment.
Overall, since this is a temp fix, I would prefer that we work with Mutex<tokio::time::Instant>. The mutex should be completely uncontended, so it should be very cheap with parking lot mutex
There was a problem hiding this comment.
If our only concern here is overflow, I'd rather just do saturating_sub. I chose system time because it's the easiest to operate on timestamps, and since there's no AtomicInstant, I preferred that over a mutex, since it makes the code way simpler, and I'd rather keep temporary fixes dead simple.
Right now we don't have a mechanism for expiring write delegation streams that disconnected, but without sending a valid Disconnect messages. That unfortunately also means that we leak libSQL connections, which are kept in-memory indefinitely. That leaks memory as well as disk space, because connections also keep temporary tables in place. FIXME: we also need to keep information about expired streams for longer, in case a client tries to send a late message. Such a message should be rejected in order to avoid running it outside of transaction context.
Right now we don't have a mechanism for expiring write delegation streams that disconnected, but without sending a valid Disconnect messages. That unfortunately also means that we leak libSQL connections, which are kept in-memory indefinitely. That leaks memory as well as disk space, because connections also keep temporary tables in place.
FIXME: we also need to keep information about expired streams for longer, in case a client tries to send a late message. Such a message should be rejected in order to avoid running it outside of transaction context.
Tested locally with
grpcurlby sending counterfeit write proxy messages that start a transaction and then disconnect: