Add insight into dual initialization errors

[mmallozzi]

Complex applications making use of a custom initializer occasionally run into issues where the implicit static initialization has been called before the explicit one has had a chance to run. Of course the solution is to ensure that the custom initializer is set up before anything else happens in the process, but in large codebases accomplishing this is sometimes more complex than it should be, and references to ThreeTenBP that trigger ZoneRulesProvider can sometimes slip through earlier in the startup sequence. When that happens, an "Already initialized" crash happens at the intentional explicit setInitializer call, but it is difficult to find the source of the implicit initialization. I propose on any initializer set, implicit or explicit, keep track of the stack trace so that a potential future "Already initialized" error can include the stack trace of the earlier call.

My proposal is to update AtomicReference<ZoneRulesInitializer> INITIALIZER to store both a ZoneRulesInitializer as well as a StackTraceElement[]. For completeness, maybe also replace AtomicBoolean INITIALIZED with an AtomicReference<StackTraceElement[]>, to keep track of both setInitializer and initialize calls as both can later cause issues. When the IllegalStateExceptions are thrown, add a new exception as its cause with the stack trace set to the appropriate saved one. It might even be best to include both in the case of duplicate initialization, where the IllegalStateException would be caused by the duplicate initialization exception which would be caused by the set initializer stack trace (if it exists).

Thoughts? This is something I can put into code form if it seems like the appropriate approach to this problem.

[jodastephen]

I can see where extra detail may help, but getting a stack trace is relatively expensive for a use case that happens very rarely. Maybe an environment variable could be added that causes a stack dump to be printed when ZoneRulesProvider is initialized. Then the exception in ZoneRulesInitializer could suggest setting the environment variable.

[mmallozzi]

That's a valid concern. From what I can tell, there are two parts to getting a stack trace in Java – obtaining it in some sort of native representation under the hood, and then translating it into Java StackTraceElement objects – and the second piece is the expensive part (ref). If we just create the exception in advance then we avoid the expensive part until and unless that exception actually needs to be set as a cause and thrown. Is that valid?

Unfortunately, the environment I'm using this in (Android) doesn't seem to have a way to statically declare environment variables so that they're set before anything happens in the process, and the OS doesn't really give any control over how the app process is launched. There are APIs to set environment variables at runtime, but if I'm already running into an issue where initializing as soon as possible isn't soon enough to preempt implicit initialization, then I'd run into the same issue with setting the environment variable at runtime.

Another approach to narrow this down might be to treat the default ServiceLoaderZoneRulesInitializer as unexpected when no zone rules are found, e.g. when someone builds with the no-tzdb variant. In that case, an exception could be thrown right at that moment. Or would Maven allow the no-tzdb variant to statically set the value of a constant boolean flag, which could disable the default initializer entirely and throw an exception later on if an explicit initializer has NOT been provided (basically inverting the situation here)?

[sschaap]

While Exception#getStackTrace() is likely an order of magnitude slower than #fillInStackTrace(), that does not mean that the latter is negligible. Looking at this 2014 post by long-time JVM developer Aleksey Shipilëv shows that Exception#fillInStackTrace() is roughly two orders of magnitude slower than exception instantiation by itself. Although the few microseconds that this would require are likely negligible in an Android app start-up sequence, such a change might have a larger impact on other TTBP users.

Back to the issue at hand, though. Whatever the size of your codebase, your Android application will normally start with a call to Application#onCreate(). If you provide a custom application class in your manifest, its #onCreate() method is a prime candidate to setup a custom zone rules initializer.

Failing that, if the SPI-based zone rules initializer runs before you can set a custom initializer, then perhaps provide a dummy ZoneRulesProvider through SPI that throws (or reports) exceptions in each of its methods. That should give you the same information that a filled-in stacktrace would.

That would require:

src/main/resources/META-INF/services/org.threeten.bp.zone.ZoneRulesProvider containing the full dotted name of your dummy zone rules provider, e.g. com.example.initializerstacktrace.TracingZoneRulesProvider and provide an implementation similar to:

public class TracingZoneRulesProvider extends ZoneRulesProvider {
    
    @Override
    protected Set<String> provideZoneIds() {
        throw new RuntimeException("using default zone rules initializer");
    }
    
    @Override
    protected ZoneRules provideRules(String regionId, boolean forCaching) {
        throw new RuntimeException("using default zone rules initializer");
    }
    
    @Override
    protected NavigableMap<String, ZoneRules> provideVersions(String zoneId) {
        throw new RuntimeException("using default zone rules initializer");
    }
}

If you set a custom initializer in time, then the tracing provider is never instantiated. Otherwise, the default initializer will create the tracing provider and will crash with an ExceptionInInitializerError.

For more information on SPI on Android, see https://developer.android.com/reference/java/util/ServiceLoader

[mmallozzi]

Good insight on the stack trace performance concerns. TracingZoneRulesProvider is a great idea, I'll give that a try. Thank you!

[mmallozzi]

@sschaap - have you had success using ServiceLoader on Android? It appears that most build systems strip out META-INF from the final binary. For what it's worth, my app builds with Buck.

https://android.googlesource.com/platform/sdk/+/jb-release/sdkmanager/libs/sdklib/src/com/android/sdklib/build/ApkBuilder.java#104

https://github.com/facebook/buck/blob/master/src/com/facebook/buck/android/MergeThirdPartyJarResources.java#L103

[sschaap]

The Android projects that I work on specifically avoid use of ServiceLoader for performance reasons. When we started using TTBP it turned out that use of ServiceLoader made Android load all dex files and scan the entire classpath, which took too much time during app startup. With the introduction of ZoneRulesInitializer.setInitializer() we now use a custom ZoneRulesInitializer that loads the tzdb.dat file from a raw resource.

That said, I'm not aware of any inherent problems in using SPI architecture on Android. As far as I know, TTBP "just works" when included as a dependency on Android. Given that TTBP uses ServiceLoader to discover ZoneRulesProvider implementations by default, that means that SPI architecture works.

Additionally, I validated the TracingZoneRulesProvider approach above in a fresh Android project before posting. This uses the default Gradle build files generated by Android Studio for new projects. While I cannot comment on Buck specifically, use of SPI on Android is definitely possible when using Gradle builds.

If you want, I can share the sandbox project for reference.

[mmallozzi]

That's good to know, sounds like not including META-INF is a Buck-specific issue then. If you could share that sandbox project that would be great.

Agreed on the performance issues with ServiceLoader on Android – I created ZoneRulesInitializer for that reason, as especially with a huge binary like ours going linearly through everything was taking forever. LazyThreeTen also helped us defer loading from resources until later in the startup sequence, although by now ThreeTenBP is used so pervasively throughout our codebase that it's probably not deferred by long anymore.

One framework which I just discovered (I've been away from the Android world for a bit) is androidx.startup, which would allow this to go into an Initializer that would run before Application.onCreate. Seems promising, I might try it out and see how peer code reviewers like it :)

[sschaap]

Sample project: https://github.com/sschaap/threetenbp-issue-146

Looking at the buck docs, you may need to add META-INF as resources for your android_library() rule (docs)

[mmallozzi]

Thanks! I missed that in the Buck docs, I'll have to play around with it to see if it can structure the directories properly. I was thrown off by java_binary having a separate meta_inf_directory argument.

[jodastephen]

Closing, as I don't think there is anything sensible for ThreeTenBp to do here.