camunda-community-hub / zeebe-client-node-js

Node.js client library for Zeebe Microservices Orchestration Engine
https://camunda-community-hub.github.io/zeebe-client-node-js/
Apache License 2.0
152 stars 38 forks source link

Emit warning when more than one job action is called for the same job #213

Closed jwulf closed 3 years ago

jwulf commented 3 years ago

The proposed type check is #210 will require the user to return in this case:

 if (listId === undefined) {
      complete.failure(`No list configuration found for ${listName}`);
        // Job failed. Execution should have stopped here, but hasn't
    }
    try {
      await this.emailService.addToList(email, listId);
      complete.success();
    } catch (e) {
      this.logError(e);
      complete.failure(e.message);
    }

That will catch the subtle bug where the addToList side-effect is executed after the job is marked as failed. The programmer intended execution to halt at that point.

Even with the type check in #210, however, it can still be written like this:

 if (listId === undefined) {
      complete.failure(`No list configuration found for ${listName}`);
      // Job failed. Execution should have stopped here, but hasn't 
      // Doesn't need to return, because the type checker sees the subsequent complete.success()
    }
    try {
      await this.emailService.addToList(email, listId);
      return complete.success(); // now returns
    } catch (e) {
      this.logError(e);
      return complete.failure(e.message); // now returns
    }

It now type-checks - all code paths return an action acknowledgement - but it still has the bug.

It would be best to make impossible states impossible to model, but I think this beyond the power of the type system.

This feature is to emit a big red warning to the logger when two job action methods are called for the same job. We can detect that this is a bug, and is definitely impossible behaviour.

I can't see a way to make it impossible to write, but we can have it make noise when it is run.