Take "true" case into consideration

[lukebakken]

Fixes #1682

[hairyhum]

I understand that the original assertion was there to fail if an operation fails on a live queue Pid.
This behaviour will retry the operation if it's failing and then fail with the timeout error.
Is there a way to make this behaviour better? Running it for 2000 times and then reporting timeout doesn't look less confusing than the failed assertion.

[lukebakken]

@hairyhum do you think the following sequence of events could trigger the badmatch:

• with/4 is called on a stopped mirrored queue here
• The thunk F fails, so we fall into the error handler here
• During that time, the queue pid becomes active
• Since the queue is mirrored, we enter this code block, and trigger a badmatch

[hairyhum]

Maybe. Mirroring does some waiting to avoid false-positives. Unfortunately F is an arbitrary function so we don't know why it fails and it's not logged.

[michaelklishin]

@lukebakken's hypothesis sounds plausible to me.

[lukebakken]

We know that retry_wait/4 is only called if F exits with one of these reasons:

noproc
noconnection
nodedown
normal
shutdown

Let me know what you think of 9d9f624. I special-cased the scenario where the queue is mirrored and the pid is alive. Then, in the other case, rather than matching against false the error handler func is called.

[hairyhum]

The fact that retry_wait is not called for every failure makes me wonder what is the difference between mirrored master and non-mirrored queue process becoming available again? I thought that the handler is called on any error.

[hairyhum]

I still don't see the difference between master process being back and the non-mirrored queue being back after disconnect.
Also because pid reuse the QPid might not be the actual queue process. In this case sleep is actually helpful because we wait for mnesia record update.

[lukebakken]

Another instance of this error: https://groups.google.com/d/topic/rabbitmq-users/u9URsZ2ova8/discussion

[lukebakken]

@hairyhum @michaelklishin @kjnilsson I'm not letting this one go! My latest change is simpler, and tries to do something sane where a crash would have happened.

[hairyhum]

This should at least give more visibility.

[lukebakken]

Thanks for the review @hairyhum

[lukebakken]

Backported to v3.7.x