binder: Dispatch transact() calls on an Executor when FLAG_ONEWAY would not be respected. #8987
Conversation
…e respected. Fixes #8914
jdcormie
changed the title
binder/src/main/java/io/grpc/binder/internal/OneWayBinderProxy.java
Outdated
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/OneWayBinderProxy.java
Outdated
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/OneWayBinderProxy.java
Outdated
Show resolved
Hide resolved
| logger.log(Level.FINEST, "A oneway transaction threw - ignoring", e); | ||
| } | ||
| }); | ||
| wrappedParcel.release(); |
There was a problem hiding this comment.
The OneWayBinderProxy contract says that callers of transact() must unconditionally call close() on 'wrappedParcel'. If we called release() inside the Runnable, the non-thread-safe wrappedParcel might be accessed concurrently from multiple threads.
The idea here is to transfer ownership to the Runnable as a raw Parcel in an Exception safe way. There are two cases. Either:
- execute() returns successfully, the Runnable promises to eventually recycle() 'parcel' in transactAndRecycleParcel(), and we release ownership of 'wrappedParcel' on the very next line.
OR - execute() throws RejectedExecutionException. In this case, the Runnable never runs and wrappedParcel.release() is never called so the caller of OneWayBinderProxy#transact() will recycle the Parcel when it calls ParcelHolder#close() in its finally.
LMK if you see a problem or have a better suggestion.
There was a problem hiding this comment.
Ah, release() is not like a close(). It steals the ownership instead of releasing the object. (The holder releases ownership because it is transferring to the caller; the caller is not releasing ownership overall which is what I'm used to with a release method, where release is similar to unref/close.)
I think this is fine then. Thank you!
Fixes #8914