-
Notifications
You must be signed in to change notification settings - Fork 372
Fix MPConfig instance caching to enable SSLSocketFactory assignment #866
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: jaredmixpanel <[email protected]>
Co-authored-by: jaredmixpanel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements instance caching for MPConfig.getInstance()
to ensure that custom SSLSocketFactory
settings are preserved and to reduce redundant object creation.
- Introduce a
ConcurrentHashMap
cache with double-check locking inMPConfig.getInstance()
- Add
clearInstanceCache()
(package-private) to reset the cache in tests - Expand Android instrumentation tests to verify caching, SSL factory consistency, and thread safety
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/main/java/com/mixpanel/android/mpmetrics/MPConfig.java | Added sInstanceCache , updated getInstance() to use caching, and added clearInstanceCache() |
src/androidTest/java/com/mixpanel/android/mpmetrics/SSLSocketFactoryTest.java | New test validating SSL factory consistency between user config and MixpanelAPI |
src/androidTest/java/com/mixpanel/android/mpmetrics/MPConfigTest.java | Added tests for default vs named instance caching |
src/androidTest/java/com/mixpanel/android/mpmetrics/IntegrationTestSSLSocketFactoryFix.java | Integration test reproducing the GitHub issue scenario |
src/androidTest/java/com/mixpanel/android/mpmetrics/ComprehensiveSSLSocketFactoryTest.java | Comprehensive tests covering caching, SSL assignment, and thread safety |
Comments suppressed due to low confidence (1)
src/main/java/com/mixpanel/android/mpmetrics/MPConfig.java:504
- [nitpick] Consider annotating this method with
@VisibleForTesting
(or equivalent) to document that it's only intended for test use.
/* package */ static void clearInstanceCache() {
simplify this caching logic by using ConcurrentHashMap.computeIfAbsent(cacheKey, key -> readConfig(appContext, instanceName)), which handles atomic lookup-and-create without explicit synchronization Co-authored-by: Copilot <[email protected]>
use the diamond operator for brevity Co-authored-by: Copilot <[email protected]>
@copilot Please update the pull request with the following change: - name: Wait for Emulator to Boot
run: |
adb wait-for-device
adb shell 'while [[ $(getprop sys.boot_completed) != "1" ]]; do sleep 1; done;' Place this immediately after the |
Co-authored-by: jaredmixpanel <[email protected]>
Added the emulator boot wait step to the Android CI workflow. The wait commands are now executed within the emulator runner script to ensure the device is fully booted before running tests. Commit: 9a46623 |
Problem
Users were unable to assign custom SSLSocketFactory using
MPConfig.getInstance()
because the method always created new instances instead of returning cached ones. This meant that when users tried to configure SSL settings:The configuration would be lost because
MixpanelAPI.getInstance()
internally calledMPConfig.getInstance()
and received a different instance.Root Cause
The issue occurred because:
MPConfig.getInstance(context, instanceName)
always calledreadConfig()
which created a new instanceMixpanelAPI
internally usedMPConfig.getInstance()
to get its configurationMPConfig
instances, so user's SSL factory settings were ignoredSolution
Implemented instance caching in
MPConfig
class:ConcurrentHashMap<String, MPConfig>
to cache instances by unique keyspackageName:instanceName
format (null instanceName becomes "default")Key Changes
Before (Broken):
After (Fixed):
Usage (Now Works!)
Tests Added
Benefits
Fixes #855.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
dl.google.com
/usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -Xmx1536M -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.11.1-bin/bpt9gzteqjrbo1mjrsomdt32c/gradle-8.11.1/lib/gradle-daemon-main-8.11.1.jar -javaagent:/home/REDACTED/.gradle/wrapper/dists/gradle-8.11.1-bin/bpt9gzteqjrbo1mjrsomdt32c/gradle-8.11.1/lib/agents/gradle-instrumentation-agent-8.11.1.jar org.gradle.launcher.daemon.bootstrap.GradleDaemon 8.11.1
(dns block)repo.gradle.org
/usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -Xmx1536M -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.11.1-bin/bpt9gzteqjrbo1mjrsomdt32c/gradle-8.11.1/lib/gradle-daemon-main-8.11.1.jar -javaagent:/home/REDACTED/.gradle/wrapper/dists/gradle-8.11.1-bin/bpt9gzteqjrbo1mjrsomdt32c/gradle-8.11.1/lib/agents/gradle-instrumentation-agent-8.11.1.jar org.gradle.launcher.daemon.bootstrap.GradleDaemon 8.11.1
(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.