Assert hit in ttl_check_interval.rs

[glademiller]

https://github.com/blackbeam/mysql_async/blob/master/src/conn/pool/ttl_check_inerval.rs#L70

right: 0', /usr/local/cargo/registry/src/github.jparrowsec.cn-1ecc6299db9ec823/mysql_async-0.22.1/src/conn/pool/ttl_check_inerval.rs:70:13

left: 160

[blackbeam]

@jonhoo, looks like this assert is the part of #65. Do you have any thoughts?

[jonhoo]

Hmm.. This suggests that there are available connections in the pool, and yet for some reason there are threads waiting for connections. That doesn't seem right at all.

@glademiller Under what circumstances did this error occur? Is it reproducible? Could you try to come up with a simple test case where the error presents itself?

[glademiller]

Any thoughts on what might be the cause. I have only seen this on an environment where the most I might be able to do is add some debug logs. My staging environment with a lot less going on hasn't hit the problem at all. In case it is important I have a pool that I clone between an actix-web service and background processing queue. If there is something I can put in the logs that would be helpful for debug please let me know.

[glademiller]

Oh another bit of context this only started occurring after I upgraded to 0.22 and setting con_ttl and the inactive connection ttl and the ttl_check_interval.

[jonhoo]

@glademiller Yeah, it makes sense you only see it in 0.22, since the assertion came with the rewrite of the pool logic in #92. I spent some time tracing through the logic of the code, and I think I know what's going on. The issue is specifically in this code:

mysql_async/src/conn/pool/recycler.rs

Lines 76 to 79 in f49716a

	exchange.available.push_back($conn.into());
	if let Some(w) = exchange.waiting.pop_front() {
	w.wake();
	}

We push to available and wake a thread that will then pick it up, but it won't pick it up yet. So if there are many futures waiting for a connection, and one becomes free, there will be a short period of time during which there is a connection in available and there are futures in waiting. @blackbeam I think the assertion is simply wrong and should be removed. We could keep a count of "notified" and try to assert using that, but it's not worth the effort.

[blackbeam]

Assert removed in cd214f1. @glademiller, is it possible to run this revision in your environment (as a git dependency)?

[blackbeam]

I've modeled Pool, Recycler and TtlCheckInterval behavior in TLA+ and only found minor issue with exchange.exist counter, so I'm closing this for now.

There is no 100% guarantee that my model matches the implementation, so feel free to reopen if needed.