TypeError: Cannot read property 'configuration of undefined' when calling stop on an interpreter that was not initialized

[AlCalzone]

Description
Recently, my tests are failing due to a crash in xstate. It seems that this happens because the interpreter was not initialized (see repro below).

Summary of all failing tests
 FAIL  packages/zwave-js/src/lib/driver/Driver.test.ts (6.342 s)
  ● lib/driver/Driver =>  › sending messages =>  › should not be possible if the driver failed to start

    TypeError: Cannot read property 'configuration' of undefined

      1222 | 
      1223 |            // First stop the send thread machine and close the serial port, so nothing happens anymore
    > 1224 |            this.sendThread.stop();
           |                            ^
      1225 |            if (this.serial != undefined) {
      1226 |                    if (this.serial.isOpen) await this.serial.close();
      1227 |                    this.serial = undefined;

      at Interpreter.stop (node_modules/xstate/lib/interpreter.js:487:20)
      at Driver.destroy (packages/zwave-js/src/lib/driver/Driver.ts:1224:19)
      at Object.<anonymous> (packages/zwave-js/src/lib/driver/Driver.test.ts:140:11)

Expected Result
No crash

Actual Result
crash

Reproduction

1. clone https://github.com/zwave-js/node-zwave-js/commit/18a000888b12143409569241e0128aac9f476499
2. run npm run test:ts -- --testPathPattern=driver/Driver.test.ts

$ npm run test:ts -- --testPathPattern=driver/Driver.test.ts

> test:ts
> jest "--testPathPattern=driver/Driver.test.ts"

  console.warn
    Warning: Attempted to read state from uninitialized service 'SendThread'. Make sure the service is started first.

      1222 | 
      1223 |            // First stop the send thread machine and close the serial port, so nothing happens anymore
    > 1224 |            this.sendThread.stop();
           |                            ^
      1225 |            if (this.serial != undefined) {
      1226 |                    if (this.serial.isOpen) await this.serial.close();
      1227 |                    this.serial = undefined;

[...]

  ● lib/driver/Driver =>  › sending messages =>  › should not be possible if the driver failed to start

    TypeError: Cannot read property 'configuration' of undefined

      1222 | 
      1223 |            // First stop the send thread machine and close the serial port, so nothing happens anymore
    > 1224 |            this.sendThread.stop();
           |                            ^
      1225 |            if (this.serial != undefined) {
      1226 |                    if (this.serial.isOpen) await this.serial.close();
      1227 |                    this.serial = undefined;

      at Interpreter.stop (node_modules/xstate/lib/interpreter.js:487:20)
      at Driver.destroy (packages/zwave-js/src/lib/driver/Driver.ts:1224:19)
      at Object.<anonymous> (packages/zwave-js/src/lib/driver/Driver.test.ts:140:11)

Test Suites: 1 failed, 1 total

Additional context

[davidkpiano]

You can mitigate this:

if (this.sendThread.initialized) {
  this.sendThread.stop();
}

Or put it in a try/catch. If you can create a simple reproducible example in CodeSandbox, that would be easier for us to work with than cloning/running a repo.

[Andarist]

So this is a funny issue... It manifests itself only if those 2 tests are "enabled" (you can just remove the rest of the file to test this out):

• https://github.com/zwave-js/node-zwave-js/blob/18a000888b12143409569241e0128aac9f476499/packages/zwave-js/src/lib/driver/Driver.test.ts#L130
• https://github.com/zwave-js/node-zwave-js/blob/18a000888b12143409569241e0128aac9f476499/packages/zwave-js/src/lib/driver/Driver.test.ts#L159

This made me suspect a shared state between those 2 because that second test fails only if the first one is executed as well.. I couldn't really find any shared state so I've added an __id property to your Driver class, caught the error, appended that this.__id to its message, and I've rethrown the error. This told me that the error that fails the test is actually the error thrown by the first of the listed tests. So why the second test fails? I believe that Jest just implements a logic similar to Cypress (and probably other testing frameworks) which is failing tests on any uncaught errors - this is good logic, often surfacing problems that were not explicitly tested against. And because your destroy method is actually an async method, the error thrown within it gets caught by Jest when it already executes other test - after all, there is no easy way to track "ownership" of an async job in JS.

If we add await to this destroy call:
https://github.com/zwave-js/node-zwave-js/blob/18a000888b12143409569241e0128aac9f476499/packages/zwave-js/src/lib/driver/Driver.test.ts#L140
we'll end up with the correct test being failed by Jest. I believe you should add this (and similar) awaits universally across the whole project in your tests - it would help you to reason about your tests better in the future.

As to the original issue - the solution proposed by @davidkpiano seems like the most appropriate one. This test that has caused all of this problem is never calling start and thus it's never calling this.sendThread.start():
https://github.com/zwave-js/node-zwave-js/blob/18a000888b12143409569241e0128aac9f476499/packages/zwave-js/src/lib/driver/Driver.test.ts#L130-L141
So its destroy doesn't really have anything to stop.

[AlCalzone]

@Andarist Oh wow, thanks for the detailed investigation. I can confirm that @davidkpiano's solution does work and I'll add awaits for the destroy calls. So I can definitely work around the issue.

However this behavior seems to have changed with an xstate update and I think that calling stop before start should not cause a crash.

[Andarist]

However this behavior seems to have changed with an xstate update and I think that calling stop before start should not cause a crash.

I don't have super strong opinion about this one. It makes sense for me both ways so kinda 🤷‍♂️ Any preference @davidkpiano ?

[AlCalzone]

To add:

calling stop before start should not cause a crash.

If it crashes (which might be desired), it should be with a clear error message like cannot stop an interpreter that was not started or something along these lines.

[davidkpiano]

We can show a warning. I'll do that. EDIT: It should suffice to just not do anything (except clean up listeners) when stopping an already stopped service.