Skip to content

Commit 096893c

Browse files
committed
fix(pool): bound release ping to prevent permit leaks
PoolConnection::drop() pings a connection before returning it to the idle queue. If the TCP peer is silently unresponsive, that ping can wait forever while retaining the pool's semaphore permit. Once all permits are stranded, subsequent connection acquisitions time out and the pool cannot recover. Bound the release-side ping with a five-second runtime-neutral timeout. Successful pings still return connections to the pool, while driver errors retain the existing hard-close behavior. On timeout, synchronously drop the floating connection without further socket I/O so DecrementSizeGuard restores the pool size and semaphore permit even if flushing or shutdown could block. Add a fake PostgreSQL server regression test that completes startup but never answers the release ping, then verifies that the pool opens a replacement. Fixes #4349
1 parent 1d674f5 commit 096893c

3 files changed

Lines changed: 143 additions & 13 deletions

File tree

Cargo.toml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,11 @@ name = "postgres"
424424
path = "tests/postgres/postgres.rs"
425425
required-features = ["postgres"]
426426

427+
[[test]]
428+
name = "postgres-pool"
429+
path = "tests/postgres/pool.rs"
430+
required-features = ["postgres", "runtime-tokio"]
431+
427432
[[test]]
428433
name = "postgres-types"
429434
path = "tests/postgres/types.rs"

sqlx-core/src/pool/connection.rs

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ use super::inner::{is_beyond_max_lifetime, DecrementSizeGuard, PoolInner};
1414
use crate::pool::options::PoolConnectionMetadata;
1515

1616
const CLOSE_ON_DROP_TIMEOUT: Duration = Duration::from_secs(5);
17+
const RETURN_TO_POOL_PING_TIMEOUT: Duration = Duration::from_secs(5);
1718

1819
/// A connection managed by a [`Pool`][crate::pool::Pool].
1920
///
@@ -311,19 +312,32 @@ impl<DB: Database> Floating<DB, Live<DB>> {
311312
// returned to the pool; also of course, if it was dropped due to an error
312313
// this is simply a band-aid as SQLx-next connections should be able
313314
// to recover from cancellations
314-
if let Err(error) = self.raw.ping().await {
315-
tracing::warn!(
316-
%error,
317-
"error occurred while testing the connection on-release",
318-
);
319-
320-
// Connection is broken, don't try to gracefully close.
321-
self.close_hard().await;
322-
false
323-
} else {
324-
// if the connection is still viable, release it to the pool
325-
self.release();
326-
true
315+
match crate::rt::timeout(RETURN_TO_POOL_PING_TIMEOUT, self.raw.ping()).await {
316+
Ok(Ok(())) => {
317+
// if the connection is still viable, release it to the pool
318+
self.release();
319+
true
320+
}
321+
Ok(Err(error)) => {
322+
tracing::warn!(
323+
%error,
324+
"error occurred while testing the connection on-release",
325+
);
326+
327+
// Connection is broken, don't try to gracefully close.
328+
self.close_hard().await;
329+
false
330+
}
331+
Err(_) => {
332+
tracing::warn!(
333+
timeout = ?RETURN_TO_POOL_PING_TIMEOUT,
334+
"timed out while testing the connection on-release",
335+
);
336+
337+
// The connection is unresponsive, so avoid all async connection I/O here.
338+
// Dropping `self` synchronously releases the pool guard and discards the socket.
339+
false
340+
}
327341
}
328342
}
329343

tests/postgres/pool.rs

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
use std::sync::{
2+
atomic::{AtomicUsize, Ordering},
3+
Arc,
4+
};
5+
use std::time::Duration;
6+
7+
use sqlx::postgres::{PgConnectOptions, PgPoolOptions, PgSslMode};
8+
use tokio::io::{AsyncReadExt, AsyncWriteExt};
9+
use tokio::net::{TcpListener, TcpStream};
10+
11+
const AUTHENTICATION_OK: &[u8] = b"R\0\0\0\x08\0\0\0\0";
12+
const BACKEND_KEY_DATA: &[u8] = b"K\0\0\0\x0c\0\0\0\x01\0\0\0\x02";
13+
const READY_FOR_QUERY: &[u8] = b"Z\0\0\0\x05I";
14+
15+
#[tokio::test]
16+
async fn return_to_pool_ping_timeout_recovers_pool_capacity() -> anyhow::Result<()> {
17+
let server = FakePostgresServer::bind().await?;
18+
let options = PgConnectOptions::new()
19+
.host("127.0.0.1")
20+
.port(server.port())
21+
.username("postgres")
22+
.database("postgres")
23+
.ssl_mode(PgSslMode::Disable);
24+
25+
let pool = PgPoolOptions::new()
26+
.min_connections(0)
27+
.max_connections(1)
28+
.acquire_timeout(Duration::from_secs(8))
29+
.test_before_acquire(false)
30+
.connect_with(options)
31+
.await?;
32+
33+
let conn = pool.acquire().await?;
34+
assert_eq!(server.connection_count(), 1);
35+
36+
drop(conn);
37+
38+
let conn = pool.acquire().await?;
39+
assert_eq!(server.connection_count(), 2);
40+
41+
conn.close().await?;
42+
pool.close().await;
43+
44+
Ok(())
45+
}
46+
47+
struct FakePostgresServer {
48+
port: u16,
49+
connection_count: Arc<AtomicUsize>,
50+
}
51+
52+
impl FakePostgresServer {
53+
async fn bind() -> std::io::Result<Self> {
54+
let listener = TcpListener::bind(("127.0.0.1", 0)).await?;
55+
let port = listener.local_addr()?.port();
56+
let connection_count = Arc::new(AtomicUsize::new(0));
57+
58+
tokio::spawn(accept_connections(listener, Arc::clone(&connection_count)));
59+
60+
Ok(Self {
61+
port,
62+
connection_count,
63+
})
64+
}
65+
66+
fn port(&self) -> u16 {
67+
self.port
68+
}
69+
70+
fn connection_count(&self) -> usize {
71+
self.connection_count.load(Ordering::SeqCst)
72+
}
73+
}
74+
75+
async fn accept_connections(listener: TcpListener, connection_count: Arc<AtomicUsize>) {
76+
while let Ok((socket, _)) = listener.accept().await {
77+
connection_count.fetch_add(1, Ordering::SeqCst);
78+
79+
tokio::spawn(async move {
80+
let _ = handle_connection(socket).await;
81+
});
82+
}
83+
}
84+
85+
async fn handle_connection(mut socket: TcpStream) -> std::io::Result<()> {
86+
read_startup_message(&mut socket).await?;
87+
88+
socket.write_all(AUTHENTICATION_OK).await?;
89+
socket.write_all(BACKEND_KEY_DATA).await?;
90+
socket.write_all(READY_FOR_QUERY).await?;
91+
socket.flush().await?;
92+
93+
let mut buf = [0_u8; 1024];
94+
95+
loop {
96+
if socket.read(&mut buf).await? == 0 {
97+
return Ok(());
98+
}
99+
}
100+
}
101+
102+
async fn read_startup_message(socket: &mut TcpStream) -> std::io::Result<()> {
103+
let mut len = [0_u8; 4];
104+
socket.read_exact(&mut len).await?;
105+
106+
let len = u32::from_be_bytes(len) as usize;
107+
let mut body = vec![0_u8; len.saturating_sub(4)];
108+
socket.read_exact(&mut body).await?;
109+
110+
Ok(())
111+
}

0 commit comments

Comments
 (0)