Not too long ago I wrote about a problem dealing with JavaScript reference variables causing problems in a broken router. The solution was (largely) to fix a Router subclass (RouterSubclass) so that its initialize() signature more correctly matched that of the Router base class. But this was not the real cause of the problem, only a solution to a symptom.
If you’ll remember, the error was only discovered through logs collected from our production code. No one had seen the error in the development environment (or paid enough attention to it if it happened). While fixing RouterSubclass helped prevent the error, it didn’t explain why we were, very rarely in production, running into situations where that error was ever able to occur in the first place.
The Problem
The error in the original code happened when RouterSubclass got an empty string as an argument. This happened as part of the overly defensive code that checked for a params.whole value and, if it didn’t exist, passed in an empty string instead.
var router = new RouterSubclass( params.whole || '' );
The inclusion of the default empty string was considered overly defensive because there really shouldn’t be any case where params.whole didn’t exist… or at least so we thought. But it must have been happening since that’s what was leading to the original error, right?
The Investigation
It was pretty clear that there must be something happening with this params.whole value that wasn’t immediately obvious - some case where it wasn’t actually getting defined. To dig deeper we started with where it got defined.
There was only one place where whole was getting defined in params and it was always run as part of the main initialization step and always before RouterSubclass was created. This fell inline with current expectations. The value assigned to it, however, came from another object. And that object got its value from a function… whose return value came from another object… and so on and so forth.
params.whole = data.whole; // but where did data.whole come from? ...
Following this trail wasn’t too terribly difficult. It could have been much worse and luckily there were only so many ways to derive this value. Even so, there was no obvious offender. The values from which this value was derived were all getting assigned as expected as part of the larger initialization task. This required some out of the box thinking.
If the problem wasn’t about assigning values, what about the unassigning of them? Turns out one of those values on which whole is based, while getting assigned correctly during initialization, at some point in time during the lifetime of the current session may also get unassigned, or more specifically assigned to null. If that happened before whole was ever set, then something bad could certainly happen.
The Cause
For better context, the code we’re talking about here is part of a web-based document editor. Users log in, starting at their home screen that lists their available documents. These documents can then be opened in the editor, and when done, users will get returned back to the home screen. Both the home screen and the document editor are part of a SPA so there are no page loads between these transitions.
When a user does transition between home and the editor, the state of the application changes. Some values are created when opening a document, then get un-created, or assigned to null, when returning to the home screen. One such value, which we’ll call dependency, was used in determining the value of our whole value.
This is where the cause of the error started to become clear. When the user opens a document from the home screen to go into the editor, dependency is assigned. When the user goes back to the home screen, dependency is unassigned. Under normal circumstances, this would look something like:
// user opens document
dependency = 'value';
// ...
params.whole = data.whole; // data.whole created from dependency
// ...
// user closes document, goes home
dependency = null;
// All ok!
But what was happening in the case where we were seeing an error was:
// user opens document
dependency = 'value';
// user quickly closes document, goes home
dependency = null;
// ...
params.whole = data.whole; // data.whole created from dependency (now null)
// Oh noes!
When a document is opened, an asynchronous initialization chain is started that includes setting dependency as well as deriving params.whole. The problem is, there’s a time gap between those two steps and if at any point the user goes back to the home screen within that time period (which has to happen basically immediately after attempting to open a document), params.whole is assigned an empty string. Despite the user going home, the document open initialization chain will continue to run through to completion, and in doing so the invalid whole value will ultimately get sent to the router even though there’s no point in even creating it at that point.
The Solution
To fix this we needed to make sure that, after any meaningful asynchronous action during initialization, a check gets made to make sure the application state was still where it was expected to be. If it wasn’t, we’d bail from the initialization chain entirely preventing anything further from running and potentially causing errors.
By “meaningful asynchronous action” I mean any gap in time where any following step could be adversely affected by a change in application state if one happened. Setting params.whole is an example of this because it depends on dependency which changes as the application state changes (going home). Another example would be for side effects. If something in editor code made changes that it shouldn’t because we weren’t really in the editor, that would also be a problem. To better clarify, consider the following, very loose representation of what our application initialization might look like:
class EditorImplementation {
async onEditorOpen (documentId) {
await app.openDocument(documentId);
const details = await app.getOpenDocument().loadDetails();
document.title = `File: ${details.title}`;
}
async onEditorClose () {
await app.closeDocument();
}
}
Imagine that some higher level manager is responsible for instantiating EditorImplementation and calling both onEditorOpen and onEditorClose as the application transitioned between the home page and editing a document. The job of EditorImplementation is to handle the editor part of the application.
During the setup in onEditorOpen(), we’re performing two asynchronous steps. The first step opens the document being loaded into the editor through app.openDocument(documentId). Once that completes, the document then becomes available through app.getOpenDocument() (which returns null when a document isn’t open). The other step loads additional details about the document that is then used to set the title of the HTML document revealing the document’s name in the user’s browser tab. The first asynchronous step is followed by a dependency on the document being open while the second is followed by a side effect that should be specific to the editor portion of the application.
Now, if a user goes back to the home screen before these any of these async tasks fully complete, we’re going to run into some trouble. For example, if a user goes home between openDocument() and getOpenDocument(), an error will occur in this line when getOpenDocument() returns null:
await app.openDocument(documentId);
// HERE: user goes home, app.closeDocument() called
const details = await app.getOpenDocument().loadDetails();
// error calling loadDetails() since getOpenDocument() now returns null!
Similarly, if a user goes home between loadDetails() and setting document.title, the code setting the title would inadvertently run while on the home screen setting the home screen’s title.
const details = await app.getOpenDocument().loadDetails();
// HERE: user goes home, now home screen is showing
document.title = `File: ${details.title}`;
// home screen title incorrectly looks like an edited file's title!
The first thing we need to do is figure out how we can determine whether or not we’re in the editor. For that we can use a flag (our application uses something a little more complex but for the example here, a flag will work fine). This flag, named isClosed, will start as false then be set to true in onEditorClose(). If at any time we see it’s value is true, we’ll know we’re no longer in the editor state of the application.
class EditorImplementation {
isClosed = false; // tracks whether editor has been closed
// ...
async onEditorClose () {
this.isClosed = true;
await app.closeDocument();
}
}
The way this flag will be checked is through a new method which, if the flag is true, will throw an error.
class EditorImplementation {
// ...
validateSession () {
if (this.isClosed) {
throw new Error('SessionError');
}
}
}
This method is called after each async call where we need to check the current application state. We then wrap all of the initialization code in a try...catch block that will be able to capture this error when it occurs. When we see it’s one of these session errors, we simply return exiting the call. If it’s any other error, it gets re-thrown so that it can be handled correctly by the manager that created and is driving EditorImplementation. The final code:
class EditorImplementation {
isClosed = false; // tracks whether editor has been closed
async onEditorOpen (documentId) {
try {
await app.openDocument(documentId);
this.validateSession(); // make sure we're still in editor
const details = await app.getOpenDocument().loadDetails();
this.validateSession();
document.title = `File: ${details.title}`;
} catch (error) {
if (error.message === 'SessionError') {
// do nothing, user aborted
return;
}
// real error, re-throw to be handled by caller
throw error;
}
}
async onEditorClose () {
this.isClosed = true;
await app.closeDocument();
}
validateSession () {
if (this.isClosed) {
throw new Error('SessionError');
}
}
}
Now, if the user closes the editor before all the async tasks in onEditorOpen() have completed, it will gracefully exit and not try to pull from any potentially non-existent values or perform any unexpected side effects.
