Skip to content

Commit

Permalink
fix(iroh-relay): do not use spawn_blocking in stun handler (#2924)
Browse files Browse the repository at this point in the history
## Description

This resulted in memory accumulation , due to blocking threads not
getting shutdown, once started. According to the docs from tokio, there
is no way to cleanup these tasks/threads, once they have been started.

From the docs:
https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html

> Be aware that tasks spawned using spawn_blocking cannot be aborted
because they are not async. If you call
[abort](https://docs.rs/tokio/latest/tokio/task/struct.JoinHandle.html#method.abort)
on a spawn_blocking task, then this will not have any effect, and the
task will continue running normally. The exception is if the task has
not started running yet; in that case, calling abort may prevent the
task from starting.
> 
> When you shut down the executor, it will wait indefinitely for all
blocking operations to finish. You can use
[shutdown_timeout](https://docs.rs/tokio/latest/tokio/runtime/struct.Runtime.html#method.shutdown_timeout)
to stop waiting for them after a certain timeout. Be aware that this
will still not cancel the tasks — they are simply allowed to keep
running after the method returns. It is possible for a blocking task to
be cancelled if it has not yet started running, but this is not
guaranteed.

The referred to limit is `512` by default, meaning with the default
thread stack size of `2MiB`, we are looking at `1GiB` of memory, simply
for keeping around these threads.

Looking at
https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.thread_keep_alive
these threads should get cleaned up after `10s`, but this does not seem
to happen. So there seems to be some unintended lingering to happen, in
either case.

## Breaking Changes

None

## Notes & open questions

This is unfortuante, but the parsing of stun packets should be fast
enough.

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
  • Loading branch information
dignifiedquire authored Nov 14, 2024
1 parent 0e57292 commit 1084400
Showing 1 changed file with 7 additions and 16 deletions.
23 changes: 7 additions & 16 deletions iroh-relay/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -484,27 +484,18 @@ async fn server_stun_listener(sock: UdpSocket) -> Result<()> {

/// Handles a single STUN request, doing all logging required.
async fn handle_stun_request(src_addr: SocketAddr, pkt: Vec<u8>, sock: Arc<UdpSocket>) {
let handle = AbortOnDropHandle::new(tokio::task::spawn_blocking(move || {
match protos::stun::parse_binding_request(&pkt) {
Ok(txid) => {
debug!(%src_addr, %txid, "STUN: received binding request");
Some((txid, protos::stun::response(txid, src_addr)))
}
Err(err) => {
inc!(StunMetrics, bad_requests);
warn!(%src_addr, "STUN: invalid binding request: {:?}", err);
None
}
let (txid, response) = match protos::stun::parse_binding_request(&pkt) {
Ok(txid) => {
debug!(%src_addr, %txid, "STUN: received binding request");
(txid, protos::stun::response(txid, src_addr))
}
}));
let (txid, response) = match handle.await {
Ok(Some(val)) => val,
Ok(None) => return,
Err(err) => {
error!("{err:#}");
inc!(StunMetrics, bad_requests);
warn!(%src_addr, "STUN: invalid binding request: {:?}", err);
return;
}
};

match sock.send_to(&response, src_addr).await {
Ok(len) => {
if len != response.len() {
Expand Down

0 comments on commit 1084400

Please sign in to comment.