Skip to content

Commit 678f804

Browse files
clrxblclrxbl
authored
fix: labrinth memory leaks (#5980)
1 parent eb9c347 commit 678f804

2 files changed

Lines changed: 63 additions & 52 deletions

File tree

‎apps/labrinth/src/database/redis/mod.rs‎

Lines changed: 54 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,19 @@ pub mod util;
2626
const DEFAULT_EXPIRY: i64 = 60 * 60 * 12; // 12 hours
2727
const ACTUAL_EXPIRY: i64 = 60 * 30; // 30 minutes
2828

29+
// Bound how many commands we send in a single Redis pipeline. The multiplexed
30+
// connection's BytesMut write buffer keeps its peak capacity for the life of
31+
// the connection, so larger pipelines cause higher steady-state RSS.
32+
const PIPELINE_CHUNK_SIZE: usize = 25;
33+
// Bound how many keys we send in a single MGET. Each MGET response must fit
34+
// into the connection's read buffer, which also retains its peak capacity. At
35+
// ~1 MB per cached value, 32 keys caps any single response at ~32 MB.
36+
const MGET_CHUNK_SIZE: usize = 32;
37+
// How long a pooled Redis connection lives before being recycled, regardless
38+
// of activity. Forced recycling is the only way to release the per-connection
39+
// BytesMut peak capacity that builds up under steady load.
40+
const REDIS_MAX_CONN_AGE: Duration = Duration::from_secs(120);
41+
2942
#[derive(Clone)]
3043
pub struct RedisPool {
3144
pub url: String,
@@ -85,14 +98,19 @@ impl RedisPool {
8598
});
8699

87100
let interval = Duration::from_secs(30);
88-
let max_age = Duration::from_secs(5 * 60); // 5 minutes
101+
let max_idle = Duration::from_secs(5 * 60); // 5 minutes
89102
let pool_ref = pool.clone();
90103
tokio::spawn(async move {
91104
loop {
92105
tokio::time::sleep(interval).await;
93-
pool_ref
94-
.pool
95-
.retain(|_, metrics| metrics.last_used() < max_age);
106+
pool_ref.pool.retain(|_, metrics| {
107+
// Drop connections that have been idle too long, OR that
108+
// are older than REDIS_MAX_CONN_AGE regardless of use.
109+
// The age-based recycle is what releases the per-connection
110+
// BytesMut peak capacity under steady traffic.
111+
metrics.last_used() < max_idle
112+
&& metrics.created.elapsed() < REDIS_MAX_CONN_AGE
113+
});
96114
}
97115
});
98116

@@ -303,13 +321,16 @@ impl RedisPool {
303321
})
304322
.collect::<Vec<_>>();
305323

306-
let v = cmd("MGET")
307-
.arg(&args)
308-
.query_async::<Vec<Option<String>>>(&mut connection)
309-
.await?
310-
.into_iter()
311-
.flatten()
312-
.collect::<Vec<_>>();
324+
let mut v = Vec::new();
325+
for chunk in args.chunks(MGET_CHUNK_SIZE) {
326+
let part = cmd("MGET")
327+
.arg(chunk)
328+
.query_async::<Vec<Option<String>>>(
329+
&mut connection,
330+
)
331+
.await?;
332+
v.extend(part.into_iter().flatten());
333+
}
313334
Ok::<_, DatabaseError>(v)
314335
}
315336
.instrument(info_span!("get slug ids"))
@@ -331,19 +352,20 @@ impl RedisPool {
331352
.map(|x| format!("{}_{namespace}:{x}", self.meta_namespace))
332353
.collect::<Vec<_>>();
333354

334-
let cached_values = cmd("MGET")
335-
.arg(&args)
336-
.query_async::<Vec<Option<String>>>(&mut connection)
337-
.await?
338-
.into_iter()
339-
.filter_map(|x| {
355+
let mut cached_values = HashMap::new();
356+
for chunk in args.chunks(MGET_CHUNK_SIZE) {
357+
let part = cmd("MGET")
358+
.arg(chunk)
359+
.query_async::<Vec<Option<String>>>(&mut connection)
360+
.await?;
361+
cached_values.extend(part.into_iter().filter_map(|x| {
340362
x.and_then(|val| {
341363
serde_json::from_str::<RedisValue<T, K, S>>(&val)
342364
.ok()
343365
})
344366
.map(|val| (val.key.clone(), val))
345-
})
346-
.collect::<HashMap<_, _>>();
367+
}));
368+
}
347369

348370
Ok::<_, DatabaseError>((cached_values, ids))
349371
}
@@ -440,6 +462,8 @@ impl RedisPool {
440462
let mut return_values = HashMap::new();
441463

442464
let mut pipe = redis_pipe();
465+
let mut pipe_cmds: usize = 0;
466+
let mut connection = self.pool.get().await?;
443467
// Doesn't need to be atomic
444468

445469
if !vals.is_empty() {
@@ -459,6 +483,7 @@ impl RedisPool {
459483
serde_json::to_string(&value)?,
460484
DEFAULT_EXPIRY as u64,
461485
);
486+
pipe_cmds += 1;
462487

463488
if let Some(slug) = slug {
464489
ids.remove(&slug.to_string());
@@ -478,46 +503,31 @@ impl RedisPool {
478503
key.to_string(),
479504
DEFAULT_EXPIRY as u64,
480505
);
481-
482-
/*
483-
if let Some(_sentinel) =
484-
cache_writers.remove(&actual_slug)
485-
{
486-
// drop it
487-
}
488-
*/
506+
pipe_cmds += 1;
489507
}
490508
}
491509

492510
let key_str = key.to_string();
493511
ids.remove(&key_str);
494512

495-
/*
496-
if let Some(_sentinel) = cache_writers.remove(&key_str)
497-
{
498-
// drop it
499-
}
500-
*/
501-
502513
if let Ok(value) = key_str.parse::<u64>() {
503514
let base62 = to_base62(value);
504515
ids.remove(&base62);
505-
506-
/*
507-
if let Some(_sentinel) =
508-
cache_writers.remove(&base62)
509-
{
510-
// drop it
511-
}
512-
*/
513516
}
514517

515518
return_values.insert(key, value);
519+
520+
if pipe_cmds >= PIPELINE_CHUNK_SIZE {
521+
pipe.query_async::<()>(&mut connection).await?;
522+
pipe = redis_pipe();
523+
pipe_cmds = 0;
524+
}
516525
}
517526
}
518527

519-
let mut connection = self.pool.get().await?;
520-
pipe.query_async::<()>(&mut connection).await?;
528+
if pipe_cmds > 0 {
529+
pipe.query_async::<()>(&mut connection).await?;
530+
}
521531

522532
drop(cache_writers);
523533

‎apps/labrinth/src/util/sentry.rs‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//
44
// TODO: PR something into sentry_actix to let us customize this
55

6-
use std::{borrow::Cow, pin::Pin, rc::Rc};
6+
use std::{borrow::Cow, pin::Pin, rc::Rc, sync::Arc};
77

88
use actix_http::{
99
StatusCode,
@@ -83,7 +83,11 @@ where
8383
}
8484

8585
fn call(&self, req: ServiceRequest) -> Self::Future {
86-
let hub = Hub::current();
86+
// Fork a Hub per request so the scope mutations below (event processor
87+
// capturing the request, span attachment) live only for this request
88+
// and are dropped when the future completes. Mutating the shared
89+
// thread-local hub instead would leak one event processor per request.
90+
let hub = Arc::new(Hub::new_from_top(Hub::main()));
8791
let client = hub.client();
8892

8993
let max_request_body_size = client
@@ -110,7 +114,6 @@ where
110114
);
111115

112116
let transaction = hub.start_transaction(ctx);
113-
transaction.set_request(sentry_req.clone());
114117
transaction.set_origin("auto.http.actix");
115118
transaction
116119
};
@@ -127,13 +130,13 @@ where
127130
sentry_req.data = Some(capture_request_body(&mut req).await);
128131
}
129132

130-
let parent_span = hub.configure_scope(|scope| {
131-
let parent_span = scope.get_span();
133+
transaction.set_request(sentry_req.clone());
134+
135+
hub.configure_scope(|scope| {
132136
scope.set_span(Some(transaction.clone().into()));
133137
scope.add_event_processor(move |event| {
134138
Some(process_event(event, &sentry_req))
135139
});
136-
parent_span
137140
});
138141

139142
let fut =
@@ -150,7 +153,6 @@ where
150153
transaction.set_status(status);
151154
}
152155
transaction.finish();
153-
hub.configure_scope(|scope| scope.set_span(parent_span));
154156
return Err(actix_err);
155157
}
156158
};
@@ -167,7 +169,6 @@ where
167169
transaction.set_status(status);
168170
}
169171
transaction.finish();
170-
hub.configure_scope(|scope| scope.set_span(parent_span));
171172

172173
Ok(res)
173174
}

0 commit comments

Comments
 (0)