breejs / bree

Bree is a Node.js and JavaScript job task scheduler with worker threads, cron, Date, and human syntax. Built for @ladjs, @forwardemail, @spamscanner, @cabinjs.
https://jobscheduler.net
MIT License
3.01k stars 78 forks source link

[bug] Job is already running #152

Closed dougg0k closed 2 years ago

dougg0k commented 2 years ago

Hi,

An issue has been happening about job not exiting or being done.

if (parentPort) {
    parentPort.postMessage("done");
} else {
    process.exit(0);
}

I also tried to import import "bthreads"; as mentioned in another thread.

Having all of these in place, and by adding the option closeWorkerAfterMs, I set it to 1minute, before that point the jobs kept sending Job "job_name" is already running, till that close happens, then afterwards all the jobs start getting online and signaling completion.

Anything that can be done here?

Btw: typescript typing for outputWorkerMetadata is wrong, there is a typo in the last word outputWorkerMetadate.

shadowgate15 commented 2 years ago

Hi, what version of bree are you using?

dougg0k commented 2 years ago

@shadowgate15 Latest which is 7.1.0.

shadowgate15 commented 2 years ago

alright I just updated to 7.1.1 which just has the typo fixes in it.bthreads was dropped in 7.0.0 so don't use that.

Are you positive that your script is finishing before it attempts to start a new job?

dougg0k commented 2 years ago

I upgraded to 7.1.1 and I removed bthreads from all jobs, now it happens the same thing, only difference is, instead of working fine after it being closed after 1min (Worker for job "job_name" exited with code 1), it continue giving the message Job "job_name" is already running afterwards.

dougg0k commented 2 years ago

One job example.

import { parentPort } from "worker_threads";
import { closeMappingJob } from "../helpers/mappingHelper";

(async () => {
    closeMappingJob(parentPort);
})();
export function closeMappingJob(workerParentPort) {
    if (workerParentPort) {
        workerParentPort.postMessage("done");
    } else {
        process.exit(0);
    }
}
shadowgate15 commented 2 years ago

I think I see the issue, give me a bit to fix it.

dougg0k commented 2 years ago

Thanks

shadowgate15 commented 2 years ago

https://github.com/breejs/bree/blob/3ba7f71fc4631bb735ddeb7b8016bf104806903f/test/issues/issue-152.js

does this test look similiar? it passes by the way.

dougg0k commented 2 years ago

Well, this is the bree instance

    const bree = new Bree({
        logger: cabin,
        root: path.join(__dirname, "../mappings"),
        defaultExtension: config.isDev ? "ts" : "js",
        acceptedExtensions: [".ts", ".js"],
        jobs: jobs.map((item) => ({ ...item, timeout: 0 })),
        outputWorkerMetadata: true,
        closeWorkerAfterMs: 1000 * 60,
    });

jobs contain name, interval and timeout.

shadowgate15 commented 2 years ago

are you using breejs/ts-worker to run the .ts files? Node workers can't run .ts files natively.

dougg0k commented 2 years ago

I am not, I am using ts-node-dev. I added the breejs/ts-worker and now this

[1640019627216] ERROR (45 on 203d97424f59):
    diagnosticText: "src/mappings/job_name.mapping.ts(7,6): error TS2695: Left side of comma operator is unused and has no side effects.\n"
    diagnosticCodes: [
      2695
    ]
shadowgate15 commented 2 years ago

yeah if you are using ts-node-dev then you shouldn't need breejs/ts-worker since ts-node-dev will do the compilation.

Is the interval on the jobs 1 min? There maybe a race condition coming in.

dougg0k commented 2 years ago

No, they are between 10 to 20 seconds randomly.

shadowgate15 commented 2 years ago

hmm... do you mind sharing a log output with us? preferably with DEBUG=bree* as an env variable.

dougg0k commented 2 years ago

Yes, here it is <removed>

If I remove closeWorkerAfterMs, they stay forever on Job "job_name" is already running.

spence-s commented 2 years ago

see: https://github.com/wclr/ts-node-dev/issues/296

@dougg0k can you see if you can reproduce with just ts-node - like in the typescript examples folder?

looks like ts-node-dev has a bunch of open issues and there is inherent weirdness using workers with ts-node anyway -- lets remove this variable first.

https://github.com/breejs/bree/blob/master/examples/typescript/package.json#L15

shadowgate15 commented 2 years ago

I think @Spence-S is correct. It doesn't look like any of the jobs are ever exiting. It might mean that the close function is never getting imported and so the worker never exits.

dougg0k commented 2 years ago

Sure, I remember that ts-node was not very good compared to ts-node-dev before. But seems that their work are ongoing.

I changed to ts-node and this is what what being returned for the jobs

/app/src/mappings/job_name.mapping.ts:1
import { parentPort } from "worker_threads";
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1031:15)
    at Module._compile (node:internal/modules/cjs/loader:1065:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at MessagePort.<anonymous> (node:internal/main/worker_thread:187:24)
    at MessagePort.[nodejs.internal.kHybridDispatch] (node:internal/event_target:562:20)
    at MessagePort.exports.emitMessage (node:internal/per_context/messageport:23:28)
Worker for job "job_name.mapping" exited with code 1
(node:38) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)

I added "type": "module" to package.json and got this for all ts files

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /app/src/index.ts
    at new NodeError (node:internal/errors:371:5)
    at Object.file: (node:internal/modules/esm/get_format:72:15)
    at defaultGetFormat (node:internal/modules/esm/get_format:85:38)
    at defaultLoad (node:internal/modules/esm/load:13:42)
    at ESMLoader.load (node:internal/modules/esm/loader:303:26)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:230:58)
    at new ModuleJob (node:internal/modules/esm/module_job:63:26)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:244:11)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}
error Command failed with exit code 1.
"dev": "cross-env NODE_ENV=development ts-node src",

tsconfig.json

"extends": "ts-node/node16/tsconfig.json",
"ts-node": {
  "files": true,
  "pretty": true
}
shadowgate15 commented 2 years ago

it you use that way I think you will need to use breejs/ts-worker or you can change the "dev" script to cross-env NODE_ENV=development TS_NODE=true NODE_OPTIONS=\"--loader ts-node/esm\" node src

dougg0k commented 2 years ago

Yeah, that's what I was trying after posting the msg, it seems to work. So, clearly the problem is with ts-node-dev.

shadowgate15 commented 2 years ago

Woah! Glad we were able to figure it out. Let us know if you need anymore help!

dougg0k commented 2 years ago

Thanks for all the help.

amitrajput1992 commented 2 years ago

Noting down what I found while working with ts-node-dev and breejs/ts-worker

ts-worker registers ts-node here https://github.com/breejs/ts-worker/blob/main/src/worker.js#L3 ts-node-dev also registers ts-node internally here https://github.com/wclr/ts-node-dev/blob/d45cf52ccf80432d9e0c3e2e998b6cfd5d8ded91/src/compiler.ts#L184

What ends up happening here is that when the code inside is job.ts file is processed by ts-node from worker.js, it has already been compiled by ts-node-dev. Basically, there can be only a single call to ts-node/register

I use ts-node-dev for local development and ts-node for a production build

Bits and pieces are taken from this issue: https://github.com/breejs/bree/issues/24

A solution that worked well for me.

function typescriptWorker() {
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const path = require("path");
  // ! ts-node is already registered in the nodemon script for dev
  const nodeEnv = process.env.NODE_ENV;
  if(nodeEnv === "production") {
    // running in production mode requires ts-node/register
    // eslint-disable-next-line @typescript-eslint/no-var-requires
    require("ts-node").register();
  }
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const workerData = require("worker_threads").workerData;
  require(path.resolve(__dirname, workerData.__filename));
}

// a typical job description:
{
    name: filename,
    path: typescript_worker,
    interval,
    worker: {
      workerData: {
        __filename: `/src/src/scheduler/jobs/${filename}.ts`,
      },
    },
  };
shadowgate15 commented 2 years ago

I thought that ts-node checked if it was already registered to prevent this. This could be an issue with the way we implement ts-node in ts-worker.