fix android http cancelation - #142
Conversation
|
| Test Suite | Result |
|---|---|
| Snapshot Tests | ✅ success |
| API Surface Check | ✅ success |
| valdi_web Integration Test | ❌ failure |
| macOS: C++ & Platform Tests | ✅ success |
| Valdi Smoke Tests | ❌ failure |
| Linux: Build Compiler | ✅ success |
| Linux: Build & Export | ✅ success |
| Linux: C++ Tests | ✅ success |
| Linux: Registry Validation | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
| Linux: Module Tests | ✅ success |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
|
failing job appears to be a timeout, not related to my changes, but I don't have permissions to re-run. |
| class DefaultHTTPRequestManager(context: Context): HTTPRequestManager() { | ||
| class DefaultHTTPRequestManager( | ||
| context: Context, | ||
| private val openConnection: (URL) -> URLConnection = { it.openConnection() }, |
There was a problem hiding this comment.
It looks like adding this defaulted Kotlin parameter removes the existing DefaultHTTPRequestManager(Context) constructor from the published JVM ABI.
I double-checked the compiled before/after classes with javap. The PR artifact only has (Context, Function1) and the synthetic default-argument constructor. a reflection test for (Context) fails with NoSuchMethodException. The in-repo Kotlin caller recompiles successfully, but external Java callers and binaries linked against the existing constructor can break.
Would it make sense to preserve the public Context constructor and keep the injectable connection factory behind an internal test seam?
There was a problem hiding this comment.
good catch! gated behind VisibleForTesting.
ran javap -p -cp bazel-bin/valdi/valdi_java_kt-kt.jar com.snap.valdi.network.DefaultHTTPRequestManager | grep -i "DefaultHTTPRequestManager(\|DefaultConstructorMarker"
and get the following:
public com.snap.valdi.network.DefaultHTTPRequestManager(android.content.Context, kotlin.jvm.functions.Function1<? super java.net.URL, ? extends java.net.URLConnection>);
public com.snap.valdi.network.DefaultHTTPRequestManager(android.content.Context);
| private val threadCount = AtomicInteger(0) | ||
|
|
||
| private val executors: ExecutorService = ThreadPoolExecutor( | ||
| MAX_CONCURRENT_REQUESTS, |
There was a problem hiding this comment.
Seems like this limits Android to four requests globally, whereas URLSession’s connection limit is per host.
I double-checked with four stalled requests to 127.0.0.1 followed by an immediately responding request using the distinct hostname localhost. The latter remained queued because all four executor workers were occupied. This means one slow host can still block unrelated hosts, unlike the HTTPMaximumConnectionsPerHost behavior referenced in the PR description.
Is the global cap intentional? If so, could we clarify the parity claim? Otherwise, would it make sense to preserve capacity across hosts?
There was a problem hiding this comment.
yeah that's my mis-read, sorry.
the connections per host difference means this is an android improvement, but not ios parity.
I looked into a few options to get something closer to ios logic. Looks like the best way if having them more closely aligned would be to use https://github.com/lysine-dev/okhttp as the default (but that's a discussion way beyond the scope of this fix - side note, I did check to see if I could set okhttp as the default in my app without any Valdi changes required via ValdiRuntimeManager.addRequestManager, but ran into a bug there - I'll raise a fix for that separately).
So, I've made a small update with tests to support the same maximum requests per hosts behaviour, with tests to the cross paltform behaviour aligns better.
…ST/PATCH cancelation
…reak existing java callers
0bfcae7 to
226f37f
Compare
Description
Android
DefaultHTTPRequestManagerdiverges from the iOS/macOS defaults in two ways:Cancellation does nothing.
HTTPRequestTask.cancel()nulls the completion but never touches the connection, so the transfer runs to completion and the result is thrown away.iOS actually cancels the
NSURLSessionTask, so it works there.Requests are serial. The executor is
ThreadPoolExecutor(0, 1, …). iOS usesNSURLSession.sharedSession, which is concurrent by default.So a cancelled request neither stops nor frees the only worker, and everything else queues behind it until it finishes on its own.
Changes
cancel()disconnects the connection (a request cancelled while queued never opens a connection).NSURLSession.HTTPMaximumConnectionsPerHost(so Android behaviour is consistent with iOS)bazel test //valdi:test_java --test_output=errorsis passing.Type of Change
Testing
bazel test //...)Testing Details
Checklist
Related Issues
Additional Context