Opts isn't Send because of LocalInfileHandlerObject

[jonhoo]

Currently, it is impossible to establish a Conn, or even just construct an Opts, in one thread, and then pass that to another, because Opts is not Send. This kind of setup is fairly common in applications using worker-pools, but is impossible with mysql_async today. In particular, it looks like Opts is not Send due to its Option<LocalInfileHandlerObject>. LocalInfileHandlerObject is not Send because the wrapped LocalInfileHandler is not Sync (Arc<T> is only Send if T: Sync + Send). I think it should be sufficient to have LocalInfileHandler require Self: Sync + Send, though I don't know if that restriction is okay to require?

[blackbeam]

Hi.

I tried to make Conn satisfy Send before LocalInfileHandler was implemented, but end up with rustc fell into recursion while trying to render compilation errors.

I'll look into it again.

[jonhoo]

Have you had any luck looking into this? It keeps coming back to bite me whenever I want to use this crate with connections in more than one thread :)

[jonhoo]

For what it's worth, Opts is indeed Send without LocalInfileHandler now: jonhoo@cbd03d6. Unfortunately, while attractive, we can't just change

https://github.com/blackbeam/mysql_async/blob/master/src/local_infile_handler/mod.rs#L71

to have : Send + Sync bounds. The impl Send for Arc would then kick in to make LocalInfileHandlerObject: Send, but the impl for WhiteListFsLocalInfileHandler would no longer work since tokio_core::Handler is neither Send nor Sync. Not sure how to get that to work without making Opts generic over T: LocalInfileHandler :/

One compromise that might work is to make Opts (and OptsBuilder) generic over T: LocalInfileHandler, but set a default with:

struct Opts<T = WhiteListFsLocalInfileHandler> where T: LocalInfileHandler {
  // ...
}

And then provide some other impl of LocalInfileHandler, say DenyLocalInfileHandler, which refuses to read any file (and since it has no inner fields, it can be Send).

[blackbeam]

I've changed WhiteListFsLocalInfileHandler to store Remote instead of Handle (see e0444a9 in sync-opts branch). Also i've added Sync + Send bounds to LocalInfileHandler (see 415c86f in sync-opts branch).

Looks like Conn and Pool should also store Remote instead of Handle to be Send + Sync. I'll try to replace it.

[jonhoo]

I think it is more okay for Conn and Pool to not be Send + Sync. Opts are often passed around to different threads, but I think that's relatively less common for the actual connections. Remote does come at a small performance overhead iirc, though maybe that doesn't matter given that we're doing I/O anyway.

[blackbeam]

Conn, which is not send, was leading to inconvenience with old hyper (hyperium/hyper#1067), but looks like it's not an issue any more.

I think it is more okay for Conn and Pool to not be Send + Sync.

Agree. Idea of using async library in multiple threads looks strange. If anybody wants to use async library in multiple threads, then he'd better to run another loop to not to pay for synchronization.

I think i'll merge sync-opts branch up to a701da4.

[jonhoo]

This probably also requires a version bump.

[blackbeam]

Yeah. Minor version will be bumped, since LocalInfileHandler::handle signature was changed. Will publish as 0.14.0.

[blackbeam]

Published 🎉