binder: Avoid potential deadlock when canceling AsyncSecurityPolicy futures

[HyunsangHan]

Move future cancellation outside of synchronized block in BinderClientTransport.notifyTerminated() to prevent deadlock if AsyncSecurityPolicy uses directExecutor() for callbacks.

Fixes #12190

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: HyunSangHan / name: HYUNSANG HAN (c54cfff, 9111970, 1c3a0ea, 4d7ed66, 0af3f85, fc77da4, 7138614)

[jdcormie]

Help me understand the change here? All those cancel() calls still appear to come from inside the @GuardedBy("this") method notifyTerminated() method ...

[HyunsangHan]

Help me understand the change here? All those cancel() calls still appear to come from inside the @GuardedBy("this") method notifyTerminated() method ...

OMG! Sorry. I realized that I missed committing the actual fix!
I've just pushed the missing commit with the proper solution.

@jdcormie Could you please check the latest commit?

[HyunsangHan]

@jdcormie
I’ve addressed the review comments. :)

[jdcormie]

Woke up this morning with a small new concern: Would this PR cause us to declare the Channel terminated before all work we've enqueued on the offload Executor is complete (or cancelled) ? Take a look at how releaseExecutors() is called right after notifyTerminated() (the site of your changes) returns. Would this need to move into the shutdown path instead?

[HyunsangHan]

Woke up this morning with a small new concern: Would this PR cause us to declare the Channel terminated before all work we've enqueued on the offload Executor is complete (or cancelled) ? Take a look at how releaseExecutors() is called right after notifyTerminated() (the site of your changes) returns. Would this need to move into the shutdown path instead?

I agree with your concern. That's a very good point.
Since cancellation is enqueued on a separate thread, there's no guarantee that previously enqueued tasks have completed by the time we declare the Channel as terminated.
To address this fundamentally, notifyTerminated should ideally only be called once those tasks have finished.

That said, while thinking about ways to improve the code, two questions came up 🤔 :

1. Because cancellation itself is enqueued onto the executor and then handled asynchronously, even if we enqueue it earlier there's still no guarantee that the cancel operation will finish before notifyTerminated is invoked. The probability might be higher, but the guarantee isn't there.
2. Looking at releaseExecutors more closely, it does attempt to gracefully release resources once queued tasks complete. However, the method itself seems to return immediately without waiting for that completion. If that's correct, then simply moving releaseExecutors before notifyTerminated wouldn't necessarily solve the problem either.

Could you elaborate a bit more on what you meant by "move into the shutdown path instead"? I want to make sure I understand your idea fully.

[jdcormie]

Regarding (1), You're right that enqueuing it earlier, in shutdown() say, doesn't solve anything. My suggestion here wasn't a good one.

Regarding (2), You're right that releaseExecutors really has nothing to do with this. We do promise not to put any new work on an Executor after releasing it, but you never proposed doing this so I don't know why I mentioned it.

I do still think we should cancel any internally scheduled Runnables and any in-flight work behind our calls to SecurityPolicy.checkAuthorization() before declaring termination. Otherwise, the application might be surprised when calling shutdownNow() on its own Executor returns one of our Runnable that it doesn't know what to do with. And if that Runnable is your proposed call to ListenableFuture#cancel which never gets run(), some AsyncSecurityPolicy resource might never get cleaned up.

The simplest way to achieve cancel() before notifyTerminate() is just to do one before the other in the same thread like today. We know calling cancel while holding a lock is problematic but putting this work on an executor is hard too because now we've got to somehow wait for it to finish, which is kind of the same problem we started with. Instead, what if we collected outstanding futures with the lock, but called cancel after releasing it? This would be similar to how shutdownInternal cleans up ongoingCalls when forceTerminate is true. Have a look at jdcormie@d0bcf4b and LMK what you think.

[HyunsangHan]

@jdcormie
Thanks for sharing your opinion!

I’ve reviewed your commit and I agree this "collect with lock, cancel without lock" pattern is a much cleaner solution. It avoids both the race condition and the deadlock scenario while staying consistent with the existing cleanup logic.

The only trade-off I see is that the logic might feel a bit less cohesive, but overall I think it’s the right direction :)

[jdcormie]

Great! Can you update your PR to take that approach and I'll merge it?

[HyunsangHan]

@jdcormie
Thanks for merging this! I really enjoyed discussing it with you. It was both fun and meaningful.