Fix for data race on `acknowledgedIncomingBytes` in class `BinderTransport`

[raulpardo]

There is a data race on the field acknowledgedIncomingBytes in class BinderTransport. As a consequence, if two threads execute handleAcknowledgedBytes concurrently the following execution could occur:

1. One thread writes reading the field acknowledgedIncomingBytes when executing this line
2. The other executing thread reads a stale/outdated value of acknowledgedIncomingBytes when executing this line.

Furthermore, this data race shows a violation of the @GuardedBy("this") annotation for the field acknowledgedIncomingBytes in class BinderTransport. The reason is that the read is not guarded by lock.

This PR proposes to solve the problem by including the read in this line within the synchronized block. This prevents the data race and ensuring that reads always read the latest written value on the field.

[linux-foundation-easycla[bot]]

• :x: - login: @raulpardo / name: Raúl Pardo . The commit (4748ad22a542810ecdaeee77f1bea166ebb6397a) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[raulpardo]

Thanks for the pointer! Indeed, if handleTransaction() cannot be executed concurrently, then there is no data race.

However, it is a bit misleading that the class is marked as @ThreadSafe, as this should imply that the class must safely handle multiple concurrent calls to handleTransaction().

[jdcormie]

However, it is a bit misleading that the class is marked as @ThreadSafe, as this should imply that the class must safely handle multiple concurrent calls to handleTransaction().

I agree with you. I haven't thought about this deeply but one idea for improvement could be to have BinderTransport stop implementing TransactionHandler directly. Instead, it could have a private inner class implement TransactionHandler (not marked @ThreadSafe) and pass that to the LeakSafeOnewayBinder constructor instead.

[kannanjgithub]

@jdcormie What is the problem with just removing @ThreadSafe from BinderTransport without changing which class implements LeakSafeOneWayBinder.TransactionHandler?

[jdcormie]

@jdcormie What is the problem with just removing @threadsafe from BinderTransport without changing which class implements LeakSafeOneWayBinder.TransactionHandler?

Because BinderTransport is intended to be thread safe from the perspective of a caller of its public methods and that's a useful thing for readers to know. And BinderTransport almost is thread safe, except its methods that implement TransactionHandler -- those are unnecessarily public.