Keep long-running tasks alive #1725
Conversation
coordinator/src/message.rs
Outdated
| let mut conn = match pool.get() { | ||
| Ok(conn) => conn, | ||
| Err(e) => { | ||
| tracing::error!("Failed to get DB connection: {e:#}"); | ||
| continue; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Maybe it would be cleaner here to wrap the inside of the while with a try block (idk if stable, we could use an async block/closure instead possibly)? that way, ? would only bubble up to that point.
There was a problem hiding this comment.
try would be super cool, but it's not stable.
I wasn't gonna bother originally, but since you mention it I can just extract the inside of the while loop into a function, like in all the other places where I fixed this.
There was a problem hiding this comment.
See this force-push diff.
There was a problem hiding this comment.
Looks good, that works too!
| let mut conn = spawn_blocking(move || pool.get()) | ||
| .await | ||
| .expect("task to complete")?; |
There was a problem hiding this comment.
Why do we spawn_blocking here but not elsewhere? Additionally, can we not use a native async connection pool if this is a big issue?
There was a problem hiding this comment.
Why do we spawn_blocking here but not elsewhere?
What do you mean by "elsewhere"? In this PR I have tried to consistently use spawn_blocking when calling pool.get().
It's very possible that we aren't doing this consistently, but we should because get blocks until the connection is retrieved or until a timeout expires.
Additionally, can we not use a native async connection pool if this is a big issue?
Yep, we can, although we didn't prioritise it thus far. It looks like we could use https://github.com/djc/bb8.
There was a problem hiding this comment.
What do you mean by "elsewhere"? In this PR I have tried to consistently use spawn_blocking when calling pool.get().
My mistake, maybe I was checking the deleted and not added part 😅
luckysori
force-pushed
the
fix/keep-tasks-alive
branch
from
7b236da
to
e0a341c
Compare
Before we would end up bubbling up the error on `pool.get()`, which would silently break out of the loop, so we would stop processin new `OrderbookMessage`s.
Before we would end up bubbling up the error on `pool.get()`, which would silently break out of the loop, so we would stop processing new `NewUserMessage`s.
Before we would end up bubbling up the error on `pool.get()`, which would silently break out of the loop, so we would stop processing new `NewUserMessage`s.
Before we would end up bubbling up the error on `pool.get()`, which would silently break out of the loop, so we would stop processin new `NewUserMessage`s.
luckysori
force-pushed
the
fix/keep-tasks-alive
branch
from
e0a341c
to
935ce61
Compare
Inspired by #1724.