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

mobile: Remove Executor from EnvoyHTTPCallbacks API #32776

Closed
wants to merge 8 commits into from

Fix flaky test

18b9a44
Select commit
Loading
Failed to load commit list.
Closed

mobile: Remove Executor from EnvoyHTTPCallbacks API #32776

Fix flaky test
18b9a44
Select commit
Loading
Failed to load commit list.
CI (Envoy) / Mobile/Android succeeded Mar 7, 2024 in 12m 43s

Mobile/Android (success)

Check has finished

Details

Check run finished (success ✔️)

The check run can be viewed here:

Mobile/Android (pr/32776/main@18b9a44)

Check started by

Request (pr/32776/main@18b9a44)

fredyw @fredyw 18b9a44 #32776 merge main@a3406aa

mobile: Remove Executor from Java & Kotlin API

This PR removes running the callbacks with the Executor in the API. Although, it makes sense to run the certain callbacks with the Executor, especially for blocking I/O so that we don't block the Envoy thread, having the API that forces the use of Executor is bad because we will always need to copy the ByteBuffer before running it with the Executor or else we may run into accessing a free-ed ByteBuffer. This change is essentially making the use of the Executor for the callbacks a user choice. Both the EnvoyHTTPCallbacks and EnvoyHTTPFilter interfaces have been updated with the comments related to using ByteBuffer data. The change also makes the Java/Kotlin API consistent with the other language APIs.

The PR is also a follow-up to #32715 to reduce the number of copies, especially for item 5 in the after section. With this change, there is only one copy from Envoy::Buffer::Instance to envoy_data.

For Cronvoy, there is no substantial change because the CronvoyUrlRequest#onData already makes a copy of the ByteBuffer passed from the callback before passing it into a separate thread. The only change is to make a copy earlier prior to spawning a thread.

Risk Level: high
Testing: CI
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: mobile

Environment

Request variables

Key Value
ref f020c18516fa28906f1b59c62e90a2be3e3a5fe7
sha 18b9a44
pr 32776
base-sha a3406aa
actor fredyw @fredyw
message mobile: Remove Executor from Java & Kotlin API...
started 1709849147.339477
target-branch main
trusted false
Build image

Container image/s (as used in this CI run)

Key Value
default envoyproxy/envoy-build-ubuntu:0ca52447572ee105a4730da5e76fe47c9c5a7c64
mobile envoyproxy/envoy-build-ubuntu:mobile-0ca52447572ee105a4730da5e76fe47c9c5a7c64
Version

Envoy version (as used in this CI run)

Key Value
major 1
minor 30
patch 0
dev true