mobile: Remove Executor from EnvoyHTTPCallbacks API

[fredyw]

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 not always a good idea because we will always need to copy the ByteBuffer before executing the callback 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 of #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. The ByteBuffer passed into the callbacks will be backed by envoy_data (outside the JVM memory management). The users are still free to copy the ByteBuffer as needed, but Envoy Mobile will no longer impose this.

For Cronvoy, there is no substantial change because Cronvoy does not use the Executor defined in the EnvoyHTTPCallbacks and 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

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #32776 was opened by fredyw.

see: more, trace.

[fredyw]

/retest

[fredyw]

/assign @abeyad @alyssawilk

[abeyad]

which thread is onResponseHeaders called from? is it the Envoy worker thread?

same question for other functions below like onResponseData

[fredyw]

It's whichever thread that calls the callback, which is probably the worker thread? The idea for the original design is to not block the Envoy main thread, which makes sense. However, this design imposes the requirement that every callback must be executed in a separate thread from within the Envoy Mobile layer, which may not always be desirable depending on the needs. This change moves the responsibility to the EnvoyHTTPCallbacks implementer. For example, with Cronvoy as an implementer of the EnvoyHTTPCallbacks, it can decide when to execute the callback in a separate thread. If you noticed, in the Cronvoy implementation, it had DirectExecutor (it is removed in this PR), which is basically a no-op Executor because Cronvoy does not want every callback to be executed in a separate thread.

[abeyad]

So the Runnable task created in CronvoyUrlRequest.onData (https://github.com/envoyproxy/envoy/pull/32776/files#diff-8f04efffbc03f667d43c68783a3480f3b0b31cc61e136a07c778e5c177aca969R876) gets executed in the thread supplied by the Executor?

[fredyw]

Before this PR, there were two Executors used in CronvoyUrlRequest:

• DirectExecutor
• mUserExecutor

The method below came from EnvoyHTTPCallbacks and the DirectExecutor implementation basically does nothing.

@Override
public Executor getExecutor() {
  return DIRECT_EXECUTOR;
}

The code in CronvoyUrlRequest.onData uses execute, which runs on mUserExecutor.

This PR removes EnvoyHTTPCallbacks::getExecutor. It won't matter for Cronvoy because returns a no-op Executor anyway for EnvoyHTTPCallbacks::getExecutor, but Cronvoy still uses mUserExecutor which is a user-supplied Executor. By not having EnvoyHTTPCallbacks::getExecutor(), Envoy Mobile no longer needs to perform a copy of the ByteBuffer before passing it to a user-defined onResponseData callback in the JvmCallbackContext.

From Envoy Mobile standpoint, we now only have Envoy::Buffer::Instance --> envoy_data and the data passed into onResponseData callback will be backed by envoy_data directly without any copy. The implementer of EnvoyHTTPCallbacks::onData can still make a copy if needed (just like the case with Cronvoy).

[alyssawilk]

so if we want to remove the enforcement that all up calls be run on the non-network thread, I'd think we'd want to ensure all current calls (unless there's exceptions) be run on the non-network thread. I don't see any changes to the non-cronvoy up calls. Are we running those all on other threads already, or would this be a major regression for all non-cronvoy E-M?

Largely I think having an invariant to not run upcalls on the network thread is a good call. Is there any other way to avoid the copy?
/wait-any

[fredyw]

I don't see any changes to the non-cronvoy up calls.

Yeah, for Cronvoy, it uses its own Executor to run on the non-network thread:

envoy/mobile/library/java/org/chromium/net/impl/CronvoyUrlRequest.java

Lines 883 to 901 in bd0130e

	Runnable task = new Runnable() {
	@Override
	public void run() {
	checkCallingThread();
	try {
	ByteBuffer userBuffer = mUserCurrentReadBuffer;
	mUserCurrentReadBuffer = null; // Avoid the reference to a potentially large buffer.
	int dataRead = data.remaining();
	userBuffer.put(data); // NPE ==> BUG, BufferOverflowException ==> User not behaving.
	if (dataRead > 0 || !endStream) {
	mWaitingOnRead.set(true);
	mCallback.onReadCompleted(CronvoyUrlRequest.this, mUrlResponseInfo, userBuffer);
	}
	} catch (Throwable t) {
	onCallbackException(t);
	}
	}
	};
	execute(task);

I don't see any changes to the non-cronvoy up calls. Are we running those all on other threads already, or would this be a major regression for all non-cronvoy E-M?

The idea is to let the users control that behavior whether or not they want to run the callbacks on the non-network thread. IOW, we don't want the library (the Java/Kotlin library in this case) to enforce that. If we look at the current implementation of onResponseData, it currently does a copy of the ByteBuffer before passing it to the Executor even though the Executor implementation maybe a direct executor (no-op), which is the case the case with the Cronvoy implementation. From the Java/Kotlin library standpoint, there's no way to know if it uses a no-op Executor, so the safest way is to always create a copy.

Before this PR, Envoy Mobile would create 2 copies:

1. JvmCallbackContext::onResponseData
2. CronvoyUrlRequest::onData

After this PR, Cronvoy will only do one copy.

I agree that this change may inconvenience the users a little bit since now they have to decide whether or not to run certain callbacks on non-network threads, but at the same time the library will no longer need to always pay the penalty of creating a copy.

Another option that I can think of is changing the EnvoyHTTPCallbacks::getExecutor to something like

Optional<Executor> getExecutor

With this change, if the Executor is empty, we don't need to create a copy. For Cronvoy, we will set it to empty. Let me know what you think.

[RyanTheOptimist]

"The idea is to let the users control that behavior whether or not they want to run the callbacks on the non-network thread." Hm, I'm not sure about this. While I can imagine that for some trivial applications this might be fine, I'm very concerned that this would be a massive footgun. I think Cronet runs callbacks on a different thread, IIRC, so it would seem like we should somehow be able to get the performance we need while preserving this invariant.

[fredyw]

Changing it to Optional<Executor> getExecutor is probably the best of both worlds? We can still default it to Optional.of(Executors.newSingleThreadExecutor()), but for Cronvoy, we will set it to Optional.empty(). When the Executor is empty, we will not need to do a copy (or the copy can be deferred to the implementer), but when the Executor is not empty, we will do a copy. What do you think?

[abeyad]

I'm usually weary of general APIs that take optional values, because most users of an API will truly treat them as optional and not look as closely at them, but this is a major decision that is being left on the user, so I think it's better not to make it optional.

Why does Cronvoy need to create it's own thread in CronvoyUrlRequest if it's already passing in an Executor to have the onResponseData function run in that thread?

To me, from the user's point of view, the ideal API would be:

onResponseData(lambda_to_run, executor_to_run_on)

That way, the invoker doesn't need to create it's own threads to run on, it just passes the executor and the lambda, and the implementation handles running the lambda on the executor with the response data.

I might be misunderstanding something.

As a side note: I agree the copying sucks so it would be nice to get rid of it, but I think the main repercussion of the copies would be memory pressure on the app. I think the effect on CPU usage and latency would be minimal.

[fredyw]

Why does Cronvoy need to create it's own thread in CronvoyUrlRequest if it's already passing in an Executor to have the onResponseData function run in that thread?

For the Cronvoy API, it wants to have control of the Executor per-request. This is not something that the current API supports. That's why Cronvoy implements a no-op Executor in the EnvoyHTTPCallbacks API.

To me, from the user's point of view, the ideal API would be:
onResponseData(lambda_to_run, executor_to_run_on)

This can simply be implemented with below without having to clutter the callback API with passing the Executor in each callback.

client.newStreamPrototype()
  .setOnResponseHeader { .... }
  .start(Optional.of(createFancyExecutor()))

By introducing an Optional<Executor>, the library does not always need to create a copy because it can know whether it needs to create a copy or not.

FTR, the default Executor in the existing code is a primitive Executor.newSingleThreadExecutor, which is good for a toy project, but might not be ideal when running on a real workload.

[abeyad]

question: we weren't running this in a separate thread from the looks of it, so why did we need copiedData?

[fredyw]

Yeah. I did this initially to be consistent with the callbacks API, but now that we have documented what the behavior is going to be with regards to the ByteBuffer, we can simply remove this logic.

[abeyad]

why did we pull these few lines out of the Runnable?

[fredyw]

The data may get destroyed if we do the copy inside the thread since the callback may complete first.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!