Introduce APIs for dependency injection of TimeZone-related information

[dkhalanskyjb]

Fixes #17
Fixes #201
Fixes #435

#611 is a space for comments, questions, and suggestions. Since the PR is at the conceptual stage, some discussion threads may end up being quite long, so the PR format is not particularly well-suited for it.

The most interesting places in the code are:

• https://github.com/Kotlin/kotlinx-datetime/blob/dkhalanskyjb/deprecate-datetime-singletons/core/common/src/TimeZoneContext.kt the new APIs.
• https://github.com/Kotlin/kotlinx-datetime/blob/dkhalanskyjb/deprecate-datetime-singletons/core/common/src/TimeZone.kt how TimeZone will look like with its static functions now deprecated. Note the updated semantics of ==, which now takes the TimeZone's origin timezone databases into account.
• https://github.com/Kotlin/kotlinx-datetime/blob/dkhalanskyjb/deprecate-datetime-singletons/timezones/full/common/src/BundledTimeZoneContext.kt the API of a separate artifact published alongside kotlinx-datetime one will be able to depend on to obtain an up-to-date timezone database.

It's important to note that this is not a full-fledged solution for providing custom timezone databases, though! Constructing a custom TimeZone is still going to be non-viable, even with the proposed API, as there's no way to construct a custom TimeZone. To properly implement that, we would (most likely) need to expose the structure of a TimeZone object (historical data, recurring rules) and the additional logic that is likely going to be needed when implementing realistic timezone databases (see

kotlinx-datetime/timezones/full/common/src/BundledTimeZoneContext.kt

Lines 84 to 96 in e550556

	private val impl: TimeZoneDatabase = FixedOffsetTimeZoneDatabase(object: TimeZoneDatabase {
	override fun get(id: String): TimeZone = getOrNull(id)
	?: throw IllegalTimeZoneException(
	"Zone ID '$id' was not recognized by the bundled timezone database (version $timeZoneDatabaseVersion)."
	)
	override fun getOrNull(id: String): TimeZone? {
	val data = zoneDataByNameOrNull(id) ?: return null
	return RuleBasedTimeZoneCalculations(readTzFile(data).toTimeZoneRules(), id, this).asTimeZone()
	}
	override fun availableZoneIds(): Set<String> = timeZones
	})

). We wanted to start small for now and watch the community response to at least the dependency injection API as a whole. Then, we would like to tweak it if necessary, and only then collect the less common use cases and expose the rest of the pieces.

[swankjesse]

This is fantastic and I appreciate how much design effort went into this.

When I set up servers and CI environments and such I always set an environment variable TZ=UTC0 so the host machine’s zone doesn’t accidentally leak through to the application layer. For example, I don’t need to worry that payment scheduling will change due to DST.

Would you consider exposing a symbol similar to TimeZoneContext.System like TimeZoneContext.Universal that is locked to UTC?

[dkhalanskyjb]

TimeZoneContext.Universal looks very specific, so probably not in the form you propose. My guess is that the API would look something like TimeZoneContext.System.withFixedCurrentTimeZone(TimeZone.UTC), though we'd need to carefully study the use cases for custom TimeZoneContexts to arrive at a good orthogonal API.

[JakeWharton]

Since this is a new API, can it use the new mechanism of default function compilation which relies on JVM default methods? This old trampoline-based version is only needed for existing APIs targeted by old code.

[dkhalanskyjb]

Good call, thanks!

[qwwdfsad]

Suggested change

	public object System: TimeZoneContext {
	public object System : TimeZoneContext {

[qwwdfsad]

(in many other places as well. ktfmt should address that en masse, feel free to ignore)

[dkhalanskyjb]

This is a weird stylistic inconsistency between functions and classes out of nowhere. Or should fun f(): Int also become fun f() : Int? In any case, yes, the entire kotlinx-datetime codebase is in need of reformatting, and I'd like to avoid hunting down all extra/missing whitespace characters by hand, it just doesn't seem worth it.

[qwwdfsad]

Is it left here by accident, or is it a place to put extensions on?

[dkhalanskyjb]

It's for extensions, yes.

[qwwdfsad]

[opt] Don't remember if we have a consistent approach to that, maybe worth leaving a KDoc?

[qwwdfsad]

Shouldn't it be requires transitive to export timezones themselves as well?

[dkhalanskyjb]

I thought about it, but then, you'd get LocalDateTime + UtcOffset + DateTimePeriod just by requiring kotlinx.datetime.zoneinfo. To me, that would look weird. I don't know what the idiomatic approach to this is, though. We're okay with such dependency transitivity in Gradle, but this is subtly different.

If someone has strong opinions, sure, let's make it transitive, I have only a weak opinion against it.

[ilya-g]

I would rather not add "transitive" to make clients that use kotlinx.datetime with bundled zoneinfo require both modules instead of just the latter.

[qwwdfsad]

Taking it's a drop-in replacement, the implementation is a bit concerning -- any get* (and if we ended up using BTZC, we probably expect a lot of usages) reads from a resource/JAR on the filesystem (also: going to be slow on Android and will cause all kinds of ANRs), then parse byte array every time.

I would expect to have some kind of thread-safe read-through cache between BTZC and the underlying resource

[dkhalanskyjb]

You are right that for repeated calls, the performance of the bundled timezone database is clearly worse than that of TimeZoneContext.System on the JVM (on the other targets, I'm assuming it should be on par). The initial call for System and Bundled is in the same ballpark (and so the ANRs are equally possible in System and Bundled at the app start).

Our general recommendation is to create TimeZone objects once and then keep a reference to them, though. If people are generally doing that in mature codebases, we can expect any cache that we introduce not to get too many hits. That would make our internal caching for Bundled strictly detrimental: it is not free, as it requires some upkeep on every call, and it can't hope to match the tailor-made caching that applications themselves introduce.

So, it's not obvious to me that caching is the right call here. We can't make it unbounded—that's too much data—so we'd need to introduce cache eviction, which would cause opaque performance spikes, require adding control knobs like system properties, etc., and still be at risk of being useless anyway.

If we measure the impact on some real-world codebases and determine that caching would be an improvement, I'll be happy to implement/accept a PR with caching. If your intuition tells you it's so obvious we don't even need measurements, I'm ready to listen, but so far, you haven't said that.

[qwwdfsad]

(context: https://github.com/Kotlin/kotlinx-datetime/pull/610/changes#r3623058277)

Especially if we want older Android devices to be one of the main consumers here, I'd expect that even with read-through cache, reading a lot of small files from the resources might be detrimental.

To workaround a similar issue, ThreeTenABP used a single Android asset: https://github.com/JakeWharton/ThreeTenABP#why-not-use-threetenbp

The discussion is quite old, though, and probably things have changed since then.

[qwwdfsad]

Potential options:

• Do nothing, not an issue because we don't care or because getResourceAsStream is fast enough
• Have a single cacheable resource. Read/parse once outside of Main thread, enjoy the rest
• Built-in asset
• Embed it as a ByteArray (See also: Deconstructing OkHttp, the trick with IDNA mappings)

[dkhalanskyjb]

Since it's possible to have a separate Android source set, we could implement a separate mechanism that is more efficient on Android. I agree this is very much worth investigating. Thank you for linking to several relevant resources and presenting a list of alternatives!

I'd like to argue that this is out of scope of this pull request. Indeed, 10+ years is a long time. Who knows—maybe Android development tooling got better at handling resources, and now they are just as efficient as the other methods? I have no idea. We'd need to ask an expert or benchmark the different approaches, but this can wait: all of this can be hidden behind the exact API this PR introduces. Let's stabilize the API first and research the performance improvements later.

[qwwdfsad]

Taking this interface is not externally implementable. I suggest making it so as to avoid compatibility burden in the future

[qwwdfsad]

I also implicitly assume that versioning of the database is a topic of further discussion (even though it can be done without this interface being publicly implemnetable)

[dkhalanskyjb]

The interface is intended to be fully externally implementable eventually, once we gather some feedback on the implementation and collect more requirements for custom timezone databases. It's unclear so far in which form we should expose the fixed-offset-timezone-database wrapper, or delegation to other timezone databases in general, etc., but once we figure that out, TimeZoneDatabase is

Even right now, implementing TimeZoneDatabase can already be useful:

val TimeZoneDatabaseWithoutDstInBerlin = object: TimeZoneDatabase {
    override fun getOrNull(id: String): TimeZone? =
        if (id == "Europe/Berlin") TimeZoneContext.System.get("UTC+01:00")
        else TimeZoneContext.System.get(id)

    override fun availableZoneIds(): Set<String> = TimeZoneContext.System.availableZoneIds()
}

This way, you can test how your code reacts to expected changes to timezone rules.

As for versioning, I'm not sure what you are referring to. Not all timezone databases have a clear version—only IANA databases do. For example, AFAIK, the Windows registry is not versioned. Fully custom timezone databases may also lack a meaningful notion of a version. If we decide to expose IANA timezone databases as a separate entity, yes, it will make sense to require providing their versions in all implementations and to implement a mechanism for combining IANA timezone databases with partial information. Right now, it's not obvious that anyone would be interested in that.

[qwwdfsad]

Could you please elaborate on what you mean here?
That it is allowed for an implementation to return the timezone for which zone.id == currentTimeZoneId() yet get(currentTimeZoneId()) throws?

[dkhalanskyjb]

Not necessarily. For example, on JS, currentTimeZoneId() is the actual time zone id, but currentTimeZone() is SYSTEM when no timezone database is provided.

[ilya-g]

Maybe name it TimeZoneDatabaseWithAddedFixedOffsets or FixedOffsetTimeZoneDatabaseWrapper? Because the current name makes an impression that it somehow converts all timezones of the inner to fixed ones.
Or perhaps an internal kdoc can help.

[ilya-g]

Do we particularly need to copy these files?
Can we use instead a task that populates tzdbDirectory itself?

[ilya-g]

I'd rather put it in internal directory so it doesn't get in sight when working with public API files.

[ilya-g]

Looks redundant after a star import (in other sample files as well)