Unsettled promises shouldn't be stored.

[alcuadrado]

When upgrading to v5 I discovered that there are multiple promises being stored, which is pretty dangerous and should be avoided. I've seen these two, but there's probably more:

• mclInitPromise in the VM
• initPromise in the Blockchain

The reason this is dangerous is that they may be rejected before they get to be handled. In the case of Blockchain, there's some code that I believe tries to avoid it, but throwing in the catch of a promise just leads to a new one that's also rejected.

This is particularly problematic in Node 15+, as unhandled promise rejections now kill the process, even if the promise was stored. For example, this leads to a crash:

class C {
  constructor() {
    this.promise = new Promise((resolve, reject) => {
      this.reject = reject;
    });
  }
}

const c = new C();
c.reject();

// This callback is meant to be executed in another event loop task
// which never gets to excecute. Node 15+ kills the process at the 
// end of a task that resulted in an unhandled promise rejection.
setImmediate(() => console.log("Here"));

How can these be done instead?

• In the case of mclInitPromise: I'd move its initialization into the opcode/precompile handlers that need them. Those are already async.
• In the case of Blockchain: Same as the VM, have an init method be called and await before every operation.

[holgerd77]

👍 I often had some uneasy feelings about this when seeing this code parts but couldn't put it into words what would have bothered me since I am not so firm with promises, thanks for this write-up. Would say that we should change here soon, we also have this pattern in some parts of the client.

[jochem-brouwer]

In both cases we could make the constructor of these classes private and instead provide a static constructor which awaits these initialization promises. (For VM we already have this, VM.create)

[alcuadrado]

Yeah, I really like that pattern. Even more than the await this.init() one.

[ryanio]

I am working on a PR to resolve this in VM and Blockchain.

Below is a working list of other places I've found with stored promises:

• devp2p:
  • Deferred class
• client:
  • VMExecution.vmPromise
  • Fetcher.in, Fetcher.out storing promises in QHeap (not sure if same concern applies here)
  • BoundProtocol.resolvers storing promises

[acolytec3]

I'm digging through the synchronizer code this morning and saw this await this.blockFetcher... promise that looks like it just sits pending until sync completes (which could notionally be forever or close to it if we're syncing mainnet). Does this count as a stored promise for purposes of this discussion?

[holgerd77]

Closed by #1811