Race condition when loading timezone files on the fly

[jwarkentin]

I'm running into a race condition where sometimes the file for a selected timezone doesn't load fast enough and timezoneJS ends up throwing a nasty invalidTZError. It doesn't happen most of the time, but it happens too much to be acceptable for a production environment.

Looking at a stack trace I see this when it happens:

invalidTZError() -- date.js (line 575)
getZone() -- date.js (line 654)
timezoneJS.timezone</this.getTzInfo() -- date.js (line 1050)
timezoneJS.Date.prototype.getTimezoneInfo() -- date.js (line 330)
timezoneJS.Date.prototype.getTimezoneOffset() -- date.js (line 321)
timezoneJS.Date.prototype.getUTCDateProxy() -- date.js (line 342)
timezoneJS.Date.prototype.getUTCMinutes() -- date.js (line 313)
timezoneJS.Date.prototype.setTimezone() -- date.js (line 471)

Glancing at the code, I don't see anything there with a callback that would allow the library to work asynchronously so it can load a file.

[DomBlack]

This bug is caused by the init function asynchronously loading the north america file. Then because the parseZones callback is executed part of the business logic requests a Date on a north american city.

loadZoneFile sets the loaded flag to true, before it even starts the async load:

            //Ignore already loaded zones.
            if (this.loadedZones[fileName]) {
                return;
            }
            this.loadedZones[fileName] = true; 
            return builtInLoadZoneFile(fileName, opts);

This means the second request sees it as already loaded. As @jwarkentin mentioned, the library isn't really designed to handle an async call. The best solution apart from a redesign of the API is to disable the async preload option.

[jwarkentin]

For the time being I've solved it by dropping this line in place to pre-load all the zone files:

timezoneJS.timezone.defaultZoneFile = timezoneJS.timezone.zoneFiles;

But obviously that's not ideal and it still doesn't guarantee no race conditions. It would be nice to have a real fix.

[mde]

Client-side access to server-side data is a long-standing problem in Web applications. Preloading is really the best solution in a prod environment, so maybe disabling the async preload is a good option. However, redesigining the API to do everything async is really not a good option either. @DomBlack, would you like to create a quickie PR to bandaid this by disallowing the async preload?

[jwarkentin]

Wouldn't it be better to provide a promise, accept a callback or fire an event when the requested files are loaded and the library is ready to provide useful results? That or make the actual date operations accept callbacks or provide promises so that the different operations can happen asynchronously and load the files they need when they need them.

That last option is the most flexible, although it would break compatibility and thus require a major version bump. I could relatively easily refactor my code to work with that. Everything else I build uses promises and events and my whole app is designed to load everything asynchronously. Honestly, the promise option would be my preference. If every date operation returned a promise (or a value when appropriate) then things would flow quite smoothly.

[jwarkentin]

Though, while not ideal, synchronous loading like this would allow it to continue working as it does now without breaking the API.

[mde]

I like the idea of the async init. I don't really have a strong opinion about the API for that, although callbacks are lowest common denominator and easier for non-promisey people to use. The actual date operations all being async seems like real overkill, and would lose the benefit of API-compatibility with the regular JS Date. This is supposed to work as a drop-in replacement, and for the most part it actually can.

I'm not a fan of full-on sync loading -- it's not very JavaScripty, and in all honestly I wish I hadn't tried to pave over the need for loaded date by attempting to load it on demand.

Maybe the constructor could throw if the lib isn't properly initialized?

[jwarkentin]

Throwing an exception if it's not initialized won't do much more than it doesn't now. When encountering the race condition there's not a lot the user of the library can do about it, even if it throws an exception. At best they would have to keep trying over and over again until it doesn't throw an exception. I think the best thing is do one of the following:

1. Load files synchronously, thus preserving the ability to have a synchronous API while not breaking things. This is not necessarily the ideal, but I think it would be acceptable.
2. Accept a ready callback to the init function that will be called when the timezone files are loaded. This gives the library user the ability to hold off on using until a time when it can know that it's safe.
3. Fire an event. I kind of prefer this over accepting a callback because it allows multiple parts of the code to be aware of the event happening. I can provide a simple, light-weight event system I've written if you want to do this. If doing this I would propose that the triggering of the event happen in it's own execution context (e.g. with setTimeout([fn], 0);) to allow other parts of the code the opportunity to actually bind to the event.

The other thing I would do, regardless, is set a ready or loaded flag that the user can check to see if it's ready, in case the event has been fired or callback has been called already. That or trigger the event on any callbacks called after the event has been triggered the first time. That's the concept of a state-based event system I once wrote.

[jwarkentin]

The more I think about it, the more I think that synchronous loading might really be the way to go. I say this because there's a lot of upfront cost to loading all the timezone files when only one or two are needed, so loading on the fly is a nice feature. Also, since I have to load them all up front when I'm trying to load the rest of my application, it introduces a lot of lag. If it could load them on the fly later when they're needed, even if loading synchronously the delay is minmal and generally not noticable to the user. While I hate to be arguing for the non-async option, I think it's the best for performance unless you're going to make the whole API asynchronous which isn't likely.

Synchronous, on-the-fly loading seems like the ideal solution for a synchronous API.

[mde]

@jwarkentin, I've added you as a contributor. Please use your good judgement fixing this issue -- just don't go full-promisey, or make the lib depend on a large, external library. Bear in mind people use this in lots of different environments, so keeping things simple is usually better. Let me know when you'd like me to push another version to NPM/Bower. :)

[jwarkentin]

Sounds good. I'll probably have time to work on it tomorrow night.