fix(gzip): rotate file uncaughtException when node is in cluster mode

[mehdibeldjilali]

Issue : #174 (comment)

[mattberther]

I'd prefer that we did not rely on error trapping to control program flow. Could we please update the PR to coordinate with the master to do the gzip when there is a cluster in use?

Thanks.

[mehdibeldjilali]

Is it OK for you now ?

[mattberther]

Does this still allow for the same type of race condition where two workers may encounter the condition at the exact same time? I still think we need to defer the task execution to the master process. I think you can do this using the node cluster module.

[mattberther]

I removed the 12 duplicate comments from the PR as well, @mehdibeldji. Not sure what happened there.

[mehdibeldjilali]

Does this still allow for the same type of race condition where two workers may encounter the condition at the exact same time? I still think we need to defer the task execution to the master process. I think you can do this using the node cluster module.

I don't think it's the best solution to use the Node.JS cluster module because, in library like PM2, the running master cluster is the cli process itself and all instances in cluster mode are Worker not Master... (c.f Unitech/pm2#2035 (comment)).

[mehdibeldjilali]

I removed the 12 duplicate comments from the PR as well, @mehdibeldji. Not sure what happened there.

Sorry, Github messages are down this morning (GMT+2)... So the behaviours was special 😄

[ankitcf]

Ideally the wdrf library should not be aware of cluster mode. The library might not be even used by a server (it could be just one off cron or script execution).

In our case, even though we are not using PM2, the master is very thin and just spawns the worker threads. It does not do any logging/execution apart from this so rotate by master will not work.

+1 for the solution proposed by @mehdibeldji

[mattberther]

I understand that this library should not be aware of cluster modes. However, since it's possible it will be run in a cluster, we have to account for it. My concerns with the way the PR currently operates are 1) what happens with log messages when multiple workers are logging messages and the proposed change is in the process of zipping, and 2) what happens when multiple workers execute this code at the same time.

It seems as though the possibility still exists to 1) miss log messages, and 2) have errors. Without any orchestration between the workers, we cannot alleviate these problems.

Or, am I missing something?

[mehdibeldjilali]

The problem is libraries or stuffs using Node.JS cluster library does not attribute Master role on an instance : all instances are considered like a Worker.
Yes, I think the second problem might append, this is the reason that I have firstly propose to catch error on fs functions...

[mattberther]

Thank you for your contribution. Merged and pushed to npm as winston-daily-rotate-file@3.3.4.