Tokio executor is assumed to be present for Conn cleanup

[PvdBerg1998]

The crate should not expect a tokio executor to be present.
Perhaps the Pool can contain an (optional) reference to an Executor, similar to how Hyper does it. This gives users the freedom to provide their own executor, or use the default one.
As far as I could see, this is the only place where a future is spawned. Without an executor present, the cleanup is never run.

[PvdBerg1998]

@blackbeam Do you need any help with this? If I have some spare time I could try to create a PR.

[blackbeam]

Hi!

Do you need any help with this?

Actually yeah, because i don't have much spare time atm. However the solution is unclear to me.

Consider this test:

mysql_async/src/conn/pool/mod.rs

Lines 536 to 544 in bfea526

	#[test]
	fn should_not_panic_if_dropped_without_tokio_runtime() {
	let pool = Pool::new(&**DATABASE_URL);
	run(collect(
	(0..10).map(|_| pool.get_conn()).collect::<Vec<_>>(),
	))
	.unwrap();
	// pool will drop here
	}

Pool here will outlive the runtime created on line 539. I believe this means that any async cleanup code
won't run, and it actually does not matter if this code works via tokio::spawn or via some in-pool queue.

Now consider the following test:

#[test]
fn disconnect_on_another_runtime() {
    let fut = Conn::new(get_opts());
    let conn = run(fut).unwrap(); // <- first runtime
    run(conn.disconnect()).unwrap(); // <- second runtime
}

Here the connection is created using the first runtime and then disconnected using the second runtime. But this test will panic with reactor gone because reactor handle is weak.

This means, i think, that asynchronous cleanup just can't be executed if Conn outlives the runtime it was created on and i don't see a sane way to enforce Conn to not to outlive it.

Approach that i tried is to unwrap raw handle from tokio TcpStream and perform synchronous cleanup, but it turns out that IntoRawFd is not implemented for tokio TcpStream (because of this, i think).

I might, however, be wrong.

Do you have any thoughts about all this?

[PvdBerg1998]

I think if the runtime is dropped before the Conn this is the fault of the user for not cleaning up their futures correctly.

I think you misunderstood my issue slightly; I'm using your lib without a tokio runtime, I use my own. However, the cleanup code is hardcoded to use the last used tokio runtime. If this is not present, it will silently fail.

My proposal is to allow a custom Executor impl to be passed (into Pool perhaps), which will then be used instead of the standard tokio one. If nothing is passed, using tokio is fine with me.

Do you agree on this? Is Pool the correct place to add this? (Didn't read into it that much yet)

[blackbeam]

I think you misunderstood my issue slightly; I'm using your lib without a tokio runtime, I use my own. However, the cleanup code is hardcoded to use the last used tokio runtime. If this is not present, it will silently fail.

Oh. This clears thing a bit. Thanks.

My proposal is to allow a custom Executor impl to be passed (into Pool perhaps), which will then be used instead of the standard tokio one. If nothing is passed, using tokio is fine with me.

Yesterday i've hoped for a blitzkrieg while adding Box<dyn Executor> into the Conn, but i've stalled deciding about Send and Sync bounds, TypedExecutor and possible implications on flexibility. I'll investigate more.

[PvdBerg1998]

Great! It may be helpful to look at hyper, since they have implemented a similar mechanism (with_executor).

[PvdBerg1998]

Any progress on this issue?