Expose underlying connection errors to user of AsyncPgConnection

[banool]

Summary

This PR addresses #118. In short, rather than just printing when there is an error with the tokio postgres connection, we expose it to the user in query path. If there is an error, they will get that rather than the inner result.

Test Plan

Set up DB:

psql postgres://dport@127.0.0.1:5432/postgres -c 'DROP DATABASE IF EXISTS diesel_test'
psql postgres://dport@127.0.0.1:5432/postgres -c 'CREATE DATABASE diesel_test'
export DATABASE_URL='postgres://dport:@127.0.0.1:5432/diesel_test'

Run tests:

cargo test --features postgres

I don't have any new tests yet. Adding a test for this behavior would be quite hard, we'd have to introduce some kind of induced failure + a way to have a deterministic order of steps. We can discuss here after we decide if the core change is good.

[weiznich]

Thanks for opening this PR 👍 It's more or less what I had in mind.

I've left a few comments about things that I would like to change. In addition we might need to make sure that we also perform that selection in SimpleConnection::batch_execute.

[weiznich]

There is no need to have this block here if we handle it in with_prepared_statement anyway.

[weiznich]

There is no need to have this block here if we handle it in with_prepared_statement anyway.

[weiznich]

Using try_recv() only gets the errors that have occurred until this function is called. It does not get us the errors that might occur while the future f is executed. That's the reason why I would prefer using select! between the channel (future) and the actual future f.

[banool]

Ah good point. I tried the snippet you suggested but tokio::select only works in an async context. To deal with that I tried to move it into the above async closure but I had difficulty with that too. Let me try the latter again.

[banool]

Hey hey, just got this working!

At first I tried this: https://gist.github.com/banool/b62cda55a4cac09c0d57e6b283b40778. This doesn't work because self is captured by the returned future. I can't clone the error receiver like we do with everything else that we feed into the future because it is not Clone.

At first I was relucant to use Arc + Mutex to solve this but I see you're doing this already in other cases. So that's what I did.

Check out the new code!

[banool]

If this looks good I can make that batch_execute change you requested too.

[weiznich]

I'm not sure if using a mutex here is a good idea, because that one would be locked while executing the query (compared to while starting to prepare as for the other mutexes). That would basically restrict the ability to do pipe lining at all. I need to think about a better solution, probably using a mpmc channel or something like that.

[banool]

Perhaps the approach is this:

1. Make a new async closure.
2. In that closure, take the lock.
3. Manually poll the future. If the future is pending we return future not ready for the closure.

I'm not 100% sure if this works but the overall idea is to make sure we only take the lock to check if the channel has an error in it and if it doesn't, let go of the lock and move on instead of blocking (so tokio::select can instead drive the other branch).

I notice that some other tokio channels maybe have an alternative form of what we need, this poll_recv function: https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.Receiver.html#method.poll_recv.

[weiznich]

I've finally found the time to have another look at this. #132 is what I've settled on.

[banool]

Neato, looks good to me! I'll close this one out.