Exception-safety of methods that temporarily mutate settings

[jasonkres]

Overview

decimal.js contains code patterns like the following to save and restore global state similar to the following. This pattern is not safe to exceptions. (One of the affected methods is cos. One of the affected properties is precision.)

var x = this,
  Ctor = x.constructor,
  pr = Ctor.precision;

// Change precision globally.
Ctor.precision = /* something */;

// Do operations under temporary precision.
// ...

// Reverse change to global precision.
Ctor.precision = pr;

This corrupts lasting global state (here, Decimal.precision) if an exception occurs under the temporary local precision. The caller may catch the exception and continue, or the script may stop but subsequent events on the web page may lead to further execution in decimal.js with unexpected precision. (And precision is not the only affected setting.)

This is not robust if a method in decimal.js has an bug that throws an exception or an unavoidable JavaScript problem occurs while the "temporary" setting is in effect (out of memory?, runtime bug?, etc.). Depending on program requirements and the value of the "temporary" setting value, this has the potential for subtle, hard-to-reproduce errors in calculations.

Example

The follow contrived example demonstrates the problem. A failure which could be contained to a single instance of Decimal corrupts the global state. A programmer who has not examined the decimal.js source code could be surprised or shocked by this.

var oldPrecision = Decimal.precision;
var d = new Decimal(1);
var x;

// We just need to simulate an exception in the "critical section" of cos.
// (Note that this assignment appears to be isolated to local instance d.
// In particular, this is not an assignment to Decimal.prototype.abs.)
d.abs = function() { throw "Corrupts global state!"; };

try {
  x = d.cos();
} catch (e) {
  // Simulate a program can continue without the value, if necessary.
  console.error(e);
  x = Number.NaN;
}

console.log(x);

if (Decimal.precision === oldPrecision) {
  console.log("GOOD");
} else {
  console.log("BAD expected Decimal.precision = " + oldPrecision + "; got " + Decimal.precision);
}

// Output:
// Corrupts global state!
// NaN
// BAD expected Decimal.precision = 20; got 28

Potential Solutions

1. try/finally

The simplest solution may be try/finally:

var x = this,
  Ctor = x.constructor,
  pr = Ctor.precision;

try {
  // Change precision globally.
  Ctor.precision = /*something*/;

  // Do operations under temporary precision.
  // ...
} finally {
  // Reverse change to global precision.
  Ctor.precision = pr;
}

2. clone

Alternatively, the method could use clone and change the settings on the clone instead of the constructor of this. The decimal value would have to be marshalled to and from that clone taking care that the returned object has the same constructor as this (which will often be Decimal).

3. Settings Object

Another design has methods take a settings object to configure details which could vary from the global defaults the end programmer has set on the Decimal constructor. (Though perhaps the methods that have the settings parameter could be private. The details that a setting object is passed around could be an internal implementation detail.)

4. Other Ideas - Breaking Changes

(Is set considered harmful?) Much of this should probably be a separate issue...

A further question is whether writable settings on a constructor are a good idea. Would immutable settings on the constructor be better? The current design may require coordination, for example if a page has multiple scripts by different developers that all try to assign settings on Decimal. Writable settings properties on the constructor seems like a headache for scripts in large projects. Woe to the script that uses decimal.js on the same page as another script that does not understand this. (Yes, there are many ways for scripts to interfere with each other in JavaScript, but that doesn't mean libraries can't be more robust.)

That said, the library has a way to create an alternate constructor via clone, which can be reset to the stock settings via set({defaults: true}). So it turns out that a cautious and well-informed programmer can avoid the pitfalls. I doubt most programmers will appreciate this. I think most would just want to write new Decimal(x) and get the same results they got in their unit tests -- regardless of what else is going on in the rest of the web page that included their script. It would be better if the settings were read-only properties of the constructor. To specify different settings, perhaps clone could take the desired settings (set would go away). As a consequence, the constructor called Decimal would always have the stock settings.

[MikeMcl]

As internal bugs cannot be completely ruled out, I agree that the library would be more exception-safe if methods did not mutate the settings, but I found that doing so was the fastest way to develop the library and it avoided the complexity and verbosity of having to pass them around and create lots of internal functions, for example, x.sqrt() versus internal_sqrt(x, precision, rounding).

One workaround for using this library in a try block would be for the user to restore the current configuration in the catch block.

Regarding having immutable settings on the constructor, the Decimal.set method allows other scripts to change the settings anyway, and ES3 compatibility means no read-only properties (unless a new ES5+ version is created that locks everything down). I could get rid of the properties, like in bignumber.js, but I would then have to add a method to retrieve their values. And I quite like just being able to do Decimal.precision = 40 rather than Decimal.set({ precision: 40 }).

The fact that all major browsers now, or will soon, support ES modules means that the problem of separate scripts modifing the state of a shared Decimal constructor is less of an issue. And, as you recognise, the constructor can always be cloned if required.

Regarding your last idea, and notwithstanding set({defaults: true}), there is nothing special about the stock settings so I wouldn't want the original Decimal to always have them.

Thanks for your analysis and apologies for my late reply. I agree that the library should be refactored so that it no longer uses the "save and restore global state" pattern. Time permitting, I will try and do that in the next update.