Fix #198

[weiznich]

This commit introduces a boolean flag that tracks whether we currently execute a transaction related SQL command. We set this flag to true directly before starting the future execution and back to false afterwards.
This enables us to detect the cancellation of such futures while the command is executed. In such cases we consider the connection to be broken as we do not know how much of the command was actually executed.

[lsunsi]

Hey the PR looks great, exactly to the point. I updated the diesel-async implementation on the reproduction repository and yes, it fixes the issue.

Personally I think this solution is great because, even though cancellation is still bad, it normalizes the behavior, which is not ideal (due to rust limitations) but it's still a step up.

[weiznich]

Thanks for testing. I will try to fix the CI next week and afterwords that should be ready to go.

[sgoll]

Would it make sense to add a debug guard against unexpectedly setting is_broken when it is already set? This should never happen intentionally but may happen unintentionally after a future refactoring—with potentially unknown consequences when we reset the value back to false below too early.

Suggested change

	is_broken.store(true, Ordering::Relaxed);
	let was_broken = is_broken.swap(true, Ordering::Relaxed);
	// We must never enter the critical block concurrently.
	debug_assert!(!was_broken);

[sgoll]

Suggested change

	is_broken.store(false, Ordering::Relaxed);
	let was_broken = is_broken.swap(false, Ordering::Relaxed);
	debug_assert!(was_broken);

[sgoll]

There is a race condition here. Without an atomic read–write instructions like swap(), you can run into the situation that two threads concurrently check the precondition—both finding it okay, i.e. false—and both then concurrently set the flag to true.

[weiznich]

That is technically true, but that cannot happen here:

• We have a &mut Conn outside of this function that ensures unique access
• We only use a Arc<AtomicBool> here to workaround borrowing multiple fields as mutable
  (* It's only a debug_assert!`)

[sgoll]

Yes, you are absolutely correct. My initial suggestion was meant as defensive programming in the sense that the &mut Conn precondition is refactored at one point in the future, and we want to guard against not having kept track of the invariant of critical_transaction_block().

To be most useful in this regard, the check would have to be made atomically. If this has too much overhead or there are other reasons for not adding the (as of now unnecessary) check, feel free to discard my remarks 🙂