Use gen_statem in Postgrex.SimpleConnection

[whatyouhide]

First pass, but seems to be quite promising. 🙃

[josevalim]

LGTM too.

[josevalim]

Generally speaking, the second element of from must be opaque and I think we are going to be in trouble if we ask users to look into it. I think we should rather tell users of this library to not rely on from and instead explicitly pass the caller too.

[josevalim]

In other words, I don't think the from should be passed on the caller here. Rather the .listen function in notifications should explicitly pass the pid.

[josevalim]

I sent a similar PR to Oban which may also be affected by this: sorentwo/oban#843

[whatyouhide]

@josevalim yes, the second element of from is opaque here outside of SimpleConnection. This is to be backwards compatible for folks using the SimpleConnection behaviour, so that they're unlikely to have to change their callbacks.

[josevalim]

That's the point. Your fix has handle_call looking at the second element of from, but there was no guarantee that would be of any shape whatsoever before this PR.

[josevalim]

Perhaps you meant to write this:

Suggested change

	# We have to do a hack here to carry the actual caller PID over to the handle_call/3
	# callback, because gen_statem uses a proxy process to do calls with timeout != :infinity.
	# This results in the caller PID not being the same as the PID in the "from" tuple,
	# so things like Postgrex.Notifications cannot use that "from"'s PID to register
	# notification handlers. This approach is paired with reconstructing the proper
	# "from" tuple in the reply/2 function in this module.
	{from_pid, from_ref} = from
	callback_from = {from_pid, {from_ref, caller_pid}}
	# We have to do a hack here to carry the actual caller PID over to the handle_call/3
	# callback, because gen_statem uses a proxy process to do calls with timeout != :infinity.
	# This results in the caller PID not being the same as the PID in the "from" tuple,
	# so things like Postgrex.Notifications cannot use that "from"'s PID to register
	# notification handlers. This approach is paired with reconstructing the proper
	# "from" tuple in the reply/2 function in this module.
	{from_pid, from_ref} = from
	callback_from = {caller_pid, from}

That would be a bit more okayish (and backwards compatible).

[whatyouhide]

Yeah jeez, that's exactly what I wanted to do. I mixed things up 🙈 Thanks for catching that!

[whatyouhide]

(and fixed)

[hkrutzer]

Will this change after erlang/otp#7081 ?

[whatyouhide]

Aaaand all tests pass 🎉

[josevalim]

@type server :: :gen_statem.server_ref() at the top?

[whatyouhide]

Sure, done ✅

[josevalim]

@whatyouhide it seems we have some heisen-failures now in the test suite. :(

[josevalim]

You can try elixir --erl "+T 9" -S mix test to see if you have better luck reproducing it locally.

[whatyouhide]

@josevalim some race conditions in the tests themselves I think, not in the gen_statem code. I sprinkled some tracing in there, and it seems like the Heisenfailingtest is gone.

[josevalim]

Do we need to sleep if we have the trace?

[whatyouhide]

I think yes as a regression. I added a comment for it too 👍

[josevalim]

I am skeptical about relying on a sleep. Perhaps tracing SC.call is the wrong thing, especially if SC.call uses another process? Perhaps it is best to assert that the connection has received a message with "SELECT 123" (picking 123 so it is distinct enough?), this way we guarantee that request has come first?

[whatyouhide]

In my tests tracing SC.call was enough. I triggered CI with just that and if it passes I think we're ok to merge that, otherwise yeah I can trace the query getting to the connection.

[whatyouhide]

I rebased on #644 and remove the direct dependency on :connection.

[josevalim]

💚 💙 💜 💛 ❤️