Conversation
Lut99
left a comment
Lut99
left a comment
There was a problem hiding this comment.
It's surprisingly drop-in at the moment, which is nice! But see my comment ~ I'm surprised the main body of the work still requires a lock and tokio::block_in_place. If it's possible to remove that, we should (although I also understand it if you want that as a separate PR to get the lib-refactor out of the door) xD
That said, maybe this is a problem of diesel rather than deadpool. Either way, I was surprised.
| debug!("Starting transaction..."); | ||
| tokio::task::block_in_place(move || { | ||
| self.conn.exclusive_transaction(|conn| -> Result<u64, Self::Error> { | ||
| self.conn.lock().unwrap().exclusive_transaction(|conn| -> Result<u64, Self::Error> { |
There was a problem hiding this comment.
I'm a little surprised that this isn't async. Is that our doing that there's a lock there? If we can remove it, that'd be lovely, then we can also get rid of the block_in_place()-call but remain in an async context.
There was a problem hiding this comment.
Okay, let's split this into two things.
I do not fully understand why this lock is there. I would have expected the connection pool to guarantee that no connection is aliased.
Besides that, I think it's not async because the underlying libs are not sync. deadpool diesel makes use of the deadpool sync library that is meant for sync interfaces. We can do away with block_in_place by using self.conn.interact(|| {}). But this simply is sugar for a thread_spawn the downside of doing that is that the captured variables need to be Sync. Not always a problem, but a guarantee I cannot make with my limited knowledge of this new project.
I did read that block_in_place can be a pain at times, although I haven't run into that problem yet.
There was a problem hiding this comment.
Do you want me to rewrite it using interact? I can add Sync bound to Self::Content I think.
There was a problem hiding this comment.
Not badly enough to warrant blocking the merge ~ but one day, probably worth it :)
There was a problem hiding this comment.
Fair enough, keep in mind that it would be a breaking change then because the trait bound becomes more restrictive.
There was a problem hiding this comment.
Since it was such a little change, I did it anyway. Let me know if you like this better, moving it back to the old variant is fine too.
This seems like it integrates nicer with async.
DanielVoogsgerd
force-pushed
the
deadpool
branch
from
d73f32f to
a30c430
Compare
Mostly syntactic sugar, but also nicely moves our tracing spans to the new thread.
DanielVoogsgerd
force-pushed
the
deadpool
branch
from
4b2d7a6 to
ec9465e
Compare
|
Looks good Daniel! And even a green checkmark across the board :) |
2 participants
This seems like it integrates nicer with async.