[fix] bree.start should throw an error if job is already started

[hdrodel]

I am a bit confused. I'm trying to dynamically start jobs with bree.start("jobName"); I wrapped it in a try/catch block in case something is not working.

try{
bree.start("jobName");
}catch(error){
myCustomLogger.error(error);
}

Now i have the problem that if the job is already running i get the message

Error: Job "test" is already started
    at Bree.start (C:\Users\administrator\Documents\Workspace\scheduler\node_modules\bree\lib    at Socket.<anonymous> (C:\Users\administrator\Documents\Workspace\scheduler\index.js:75:10)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

The try/catch block does not trigger! i checked out the "start" function on row 400 here it looks like first it checks if a job with this name exists and trows an error if not and then it checks for already running jobs but then it does something different:

  return this.config.logger.warn(
          new Error(`Job "${name}" is already started`)
        );

does the error need to be thrown in order to trigger a try/catch? is this behavior intentional? I expected my try/catch to trigger when there's already a job running with this name.

[shadowgate15]

We don't throw the error so try/catch won't be called. it just returns undefined. we should probably have this return some sort of confirmation that it was scheduled.

In this case we should probably also have it throw, maybe? Thoughts @niftylettuce?

[climba03003]

I am actually against this proposal. I do not expect the cronjob should break the main thread if it can not be run.

I think return an boolean to indicate is the job started by this call should be enough.

[hdrodel]

my opinion is that the handling should be consistent. Right now if a job can't be started because it does not exist, an error is thrown. If it can't be started because it's already running, something is printed into the console.

if bree.start() is called without try/catch and the job name is wrong, you get an unhandled promise rejection which in future versions will exit with a non zero code.

without removing the throwing of the error on a invalid job name, try/catch is needed anyhow. so it does not matter if the error when starting a already running job is also thrown.

just returning a boolean will make hard to find out why the job was not created. i think an option would be something like an error first callback:

bree.start("jobTest",(error)=>{
if(error){
  //handle error here
}
});

[niftylettuce]

PR welcome