Use Lock.lockInterruptibly only where it may actually be needed

[stIncMale]

This PR reverts most of the behavioral changes done in #1206.

JAVA-5189

[rozza]

Only one naming nit - other than that LGTM.

I ran the slow tests and the build is happy again :)

[rozza]

Naming nits: Just for clarity and to mirror the lock method names, I recommend:

withLock - should use lock.lock()
withLockInterruptibly - should use lock.lockInterruptibly

[jyemin]

I agree with this, both for consistency with the Lock interface, and also because the negative "un" increases my cognitive load when reasoning about it. Also, I think for locking what we're saying is that we should lock uninterruptibly "by default", and only lock interruptibly when when we have a reason. So having a simple name for the default case is a good thing.

[stIncMale]

I also agree with this naming, but I had to make the names the way they are because the original methods that were in Locks (withLock, checkedWithLock) use interruptible locking. So to use the naming scheme proposed by Ross (it's also consistent with the naming in the Lock API), I will have to rename the original Locks.withLock/checkedWithLock methods, which will affect the code that have not yet been touched either by #1206 or by the current PR. If this is OK, I will be glad to do the renaming.

[jyemin]

It's ok. Proceed.

[stIncMale]

Done. The rename was automatic, followed by semi-manual "import the method statically" in places where it was possible (IntelliJ does not preserve static imports when refactoring).

[jyemin]

Now that we have a mix of interruptible and "normal" locking, when I look at all the places where they are used, I'm not always sure which is right. For example, I'm not clear why ClusterClock uses interruptible locking. Is there a rule of thumb that we can state?

[jyemin]

I agree with this, both for consistency with the Lock interface, and also because the negative "un" increases my cognitive load when reasoning about it. Also, I think for locking what we're saying is that we should lock uninterruptibly "by default", and only lock interruptibly when when we have a reason. So having a simple name for the default case is a good thing.

[stIncMale]

I ran the slow tests and the build is happy again :)

Yeah, I also included them in the patch created by GitHub for this PR. Additionally, here are the results of running everything: https://spruce.mongodb.com/version/651f7a3d2a60ed945c2fdff4/tasks?page=0&sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC (the only failures are of the AggregatesTest, which is fixed by #1218.

[stIncMale]

Now that we have a mix of interruptible and "normal" locking, when I look at all the places where they are used, I'm not always sure which is right. For example, I'm not clear why ClusterClock uses interruptible locking. Is there a rule of thumb that we can state?

We had this mix before doing changes in #1206 and the current PR, i.e., we are not in a new situation.

I'm not clear why ClusterClock uses interruptible locking

Neither am I, it does not need to use interruptible locking, if we talk about using interruptible locking conservatively. But it has already been like this, and things were working, so I did not change it.

Is there a rule of thumb that we can state?

Here are my thoughts at the moment, but take them with a grain of salt (also, they hardly can be expressed as an algorithm, at best they can be rules of thumb, as you said):

Interruptible locking is likely not needed:

1. If code in a critical section is either non-blocking, or the code is blocking, but while blocking it releases the lock guarding the critical section (for example, Condition.await). Sometimes we still have to "violate" this rule, like in a few places in the connection pool, just because there is at least one pre-existing test that fails if locking is not interruptible.
2. If code in a critical must be executed no matter what (unless the process is about to be terminated). For example, interruptible locking is likely harmful if a critical section completes a callback, or if it is the body of a finally block.

Update: Item 2 is what put us in trouble, and is why I had to start using interruptible locking conservatively. Despite interruptible locking not being harmful in situations described by item 1, it is not always that obvious whether the code cannot be also part of a situation from item 2 (one would have to check all the current usages and think about possible future uses). Consequently, I crawled into the "don't use interruptible locking unless it's actually needed, even if it appears to be acceptable and not harmful" corner out of caution/fear.

[jyemin]

I'm not clear why ClusterClock uses interruptible locking

Neither am I, it does not need to use interruptible locking, if we talk about using interruptible locking conservatively. But it has already been like this, and things were working, so I did not change it.

I did it in scope of JAVA-4642. I don't think it does any harm, but it does seem unnecessary.

[jyemin]

LGTM

[stIncMale]

Will merge, because @rozza LGTMed with the condition that the methods are renamed, and they were renamed.