Skip to content

binder: Rationalize @ThreadSafe-ty of BinderTransport. #12130

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

Merged
merged 3 commits into from
Jun 6, 2025

Conversation

jdcormie
Copy link
Member

@jdcormie jdcormie commented Jun 5, 2025

  • Use @BinderThread to document thread access rules for methods and fields.
  • Make TransactionHandler impl non-public since only Android should call it.
  • Replace an unnecessary AtomicLong with a plain old long.
  • Restructure sendAcknowledgeBytes() so the ack tracking fields are checked and updated in the same place (caller)

Fixes #12047

jdcormie added 2 commits June 4, 2025 17:30
- Use @BinderThread to document restrictions on methods and certain fields.
- Make TransactionHandler non-public since only Android should call it.
- Replace an unnecessary AtomicLong with a plain old long.
@jdcormie jdcormie changed the title Thread safe binder transport binder: Rationalize @ThreadSafe-ty of BinderTransport. Jun 5, 2025
@jdcormie jdcormie requested a review from shivaspeaks June 5, 2025 00:45
@jdcormie jdcormie marked this pull request as ready for review June 5, 2025 00:51
Copy link
Member

@shivaspeaks shivaspeaks left a comment

Choose a reason for hiding this comment

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

I'll buy what you're saying, just the question to understand the intent of that change of synchronized block.

CC @ejona86

@jdcormie
Copy link
Member Author

jdcormie commented Jun 6, 2025

See http://fusion2/7329b6cc-a124-44d8-8428-66b2a0b3773b for flow control stress test run. Results are neutral: http://perfgate/analysis-results?run_key=5468791659429888

@jdcormie jdcormie merged commit c206428 into grpc:master Jun 6, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify thread safety of BinderTransport and its TransactionHandler implementation
2 participants