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]

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

[jdcormie]

Perhaps this should be clearer in the code but all incoming binder transactions are FLAG_ONEWAY so Android guarantees only one thread (from the process' binder thread pool) is calling handleTransaction() at a time.

https://github.com/grpc/grpc-java/blob/master/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java#L49-L51

So every read and write of acknowledgedIncomingBytes "happens-before" the last.

Given this context, do you still believe there is a race?

[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.