The transaction id handling is unleashing Zalgo

[joelpurra]

It might seem contrived, but it's showing that Zalgo is indeed unleashed.

• Create a context.
• Start a lookup, for example using ctx.address(), with a callback (expected to be called asynchronously).
• Store the transaction id (which is a synchronous operation): const transId = ctx.address(name, callback)
• Synchronously call const cancelSuccess = context.cancel(transactionId) (which also is a synchronous operation).
• Now the lookup callback will also be called synchronously.

During testing, and possibly in the wild, the callback being called means the lookup completed, the test is over, and the context can be destroyed. This unfortunately also prevents further synchronous and asynchronous expectations from being checked (or at least from being reported). This means it's hard to tell if/how/where the code will continue to execute.

The following test is written to show that a transaction should not be able to be cancelled twice. The first call to cancel() should return true, second call should false. The second cancel() call is never invoked, within the scope of the test. Outside of the scope of collecting test results it might be called though -- with or without side-effect. (The callback might also inadvertently be called twice, which is a big node.js no-no.) The test passes, despite the synchronous call to expect().fail().

// Take from the tests, executed using mocha.
it("Should not be able to synchronously cancel the query twice", function(done) {
    const ctx = getdns.createContext({
        resolution_type: getdns.RESOLUTION_STUB,
    });

    let callCount = 0;

    const transId = ctx.address("nlnetlabs.nl", (err, result) => {
        // TODO BUG: this callback is being called synchronously due to cancel() being synchronous.
        // TODO BUG: the callback might be called twice. This might not be detectable after the test has ended.
        callCount++;
        expect(callCount).to.be(1);

        expect(err).to.be.an("object");
        expect(result).to.be(null);
        expect(err).to.have.property("msg");
        expect(err).to.have.property("code");
        expect(err.code).to.equal(getdns.CALLBACK_CANCEL);
        shared.destroyContext(ctx, done);
    });

    expect(transId).to.be.ok();

    const cancelResult = ctx.cancel(transId);

    // TODO BUG: this code is never reached, at least not within the scope of the test.
    expect().fail("Something is wrong, because this is never called (or at least not reported).");

    expect(cancelResult).to.be.ok();

    // NOTE: should not be able to cancel the same transaction twice.
    const cancelResult2 = ctx.cancel(transId);
    expect(cancelResult2).to.not.be.ok();
});

---

Unleasing Zalgo means that work might perform work synchronously, or it might performed asynchronously. With connected functions, such as those on the context object, all public functions should be either synchronous or asynchronous. As the lookups rely on callbacks, all functions need to be asynchronous.

• Returning a synchronous value from a function call accepting a callback is out of the ordinary for a node.js API, and a bad sign.
  • const transId = ctx.address(name, callback).
• Having a function which returns a synchronous value also synchronously invoke a callback is bad.
  • const cancelSuccess = context.cancel(transactionId).
• There might be similar sync/async issues when creating destroying the context. Sync/async destruction issues seems to have been handled in getdns.js as a workaround.
• The quick fix is to make at least context.cancel(transactionId) somehow invoke the callback asynchronously.
• The better solution is to make the entire API asynchronous.
  • Promises are the current preferred way to handle a mix of synchronous and asynchronous work, and also allow a natural way forward towards async/await.
  • In the case of canceling lookups, Bluebird promises with cancellation support might be a good alternative.

It will most likely require a bunch of work to redesign the getdns-node api to be consistent and more node.js-like. The question is if the current style should be accessible (to be similar to the getdns API for the same of being similar) or if this should be a node.js API built on top of the C API getdns. Any comments?

• Current issues (possibly) include:
  • Creating the context, if some internal initialization is done asynchronously.
  • Returning the transaction id from a lookup.
  • Canceling a transaction using the transaction id.
  • Destroying the context, if any internal work is done asynchronously.