Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

binder: Dispatch transact() calls on an Executor when FLAG_ONEWAY would not be respected. #8987

Merged
merged 2 commits into from
Mar 17, 2022

Conversation

jdcormie
Copy link
Member

Fixes #8914

@jdcormie jdcormie requested a review from markb74 March 15, 2022 23:01
@jdcormie jdcormie changed the title Dispatch transact() calls on an Executor when FLAG_ONEWAY would not be respected. binder: Dispatch transact() calls on an Executor when FLAG_ONEWAY would not be respected. Mar 15, 2022
Copy link
Contributor

@markb74 markb74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this!

@jdcormie jdcormie merged commit 2bf0a1f into master Mar 17, 2022
temawi pushed a commit to temawi/grpc-java that referenced this pull request Apr 8, 2022
logger.log(Level.FINEST, "A oneway transaction threw - ignoring", e);
}
});
wrappedParcel.release();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdcormie, @markb74, this release() is suspicious. Should it be done within the executor instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. execute() returns successfully, the Runnable promises to eventually recycle() 'parcel' in transactAndRecycleParcel(), and we release ownership of 'wrappedParcel' on the very next line.
    OR
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@ejona86 ejona86 deleted the transact-dispatch branch April 22, 2022 19:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

binder: Deadlock due to unexpected re-entrancy of transactions on process-local Binder
3 participants