winstonjs / winston-daily-rotate-file

A transport for winston which logs to a rotating file each day.
MIT License
889 stars 151 forks source link

Prevent crash in bad timing cases #366

Closed Apollon77 closed 7 months ago

Apollon77 commented 1 year ago

Resolves error cases like

ENOENT: no such file or directory, unlink '/opt/iobroker/log/iobroker.2023-01-04.log'
Error: ENOENT: no such file or directory, unlink '/opt/iobroker/log/iobroker.2023-01-04.log'
    at Object.unlinkSync (node:fs:1767:3)
    at WriteStream.<anonymous> (/opt/iobroker/node_modules/winston-daily-rotate-file/daily-rotate-file.js:140:28)
    at WriteStream.emit (node:events:525:35)
    at WriteStream.emit (node:domain:489:12)
    at finish (node:internal/streams/writable:756:10)
    at finishMaybe (node:internal/streams/writable:741:9)
    at afterWrite (node:internal/streams/writable:506:3)
    at onwrite (node:internal/streams/writable:479:7)
    at node:internal/fs/streams:416:5
    at FSReqCallback.wrapper [as oncomplete] (node:fs:816:5)

In rare cases it can happen that with multiple processes the archiving is started several times and so the "cloanup" is than also triggered overlapping. And in rare cases it can happen that the existsSync returns true but then for delete the file is gone already. We already fixed several such topics in the past

Apollon77 commented 1 year ago

@mattberther Is there any chance to get that merged and a new version released please? We prepare the update of new ipBroker js.controller core component and I would like to include the fix there.

Apollon77 commented 1 year ago

@mattberther @wbt Any chance?

wbt commented 1 year ago

It looks good to me, but I don't have release privileges on this package.

Apollon77 commented 1 year ago

@mattberther @indexzero Maybe you can help?

vbasset-repam commented 1 year ago

+1

Apollon77 commented 1 year ago

@mattberther any change to get that into a release? We are just about to release new iobroker version eher I would love to fix this issue.

vbasset-repam commented 1 year ago

image

@mattberther This is really important for us too, this error crash all our services... 🥲

foxriver76 commented 1 year ago

@wbt

It looks good to me, but I don't have release privileges on this package.

Can you ping someone who has? Would be much appreciated.

DABH commented 1 year ago

I think it’s just @mattberther (though Matt, if you want to add folks like me and @wbt to the npm repo, we could be available as backups to run npm releases for the project…).

Apollon77 commented 1 year ago

we from ioBroker (@foxriver76 or me) would also be available :-)

wbt commented 1 year ago

@mattberther any change to get that into a release? We are just about to release new iobroker version eher I would love to fix this issue.

I think there is a way to pin the dependency to a specific GitHub hash in your release if that's the only holdup.

Apollon77 commented 1 year ago

I think there is a way to pin the dependency to a specific GitHub hash in your release

Yes but this also requires all users tzo have git installed or getting an error on installation, so honestly no real option

Apollon77 commented 7 months ago

@mattberther Any chance for a merge? Else we slowly need to think about forking it "officially" ... SO I repeat my offer my/our support to maintain the repo

mattberther commented 7 months ago

@Apollon77 im happy to begin the process of adding maintainers to this. As you can see, I’m having a hard time keeping on with the work on this project.

Please open a new issue and we can move the discussion there.

Apollon77 commented 7 months ago

I will do another pass through the code for comparable places and also fix them "the same way" (o know there are some) ... then we can do a review @foxriver76 ... (and other interested). Then we release one new version

Apollon77 commented 7 months ago

@mattberther @foxriver76 Ok I had another look and I would propose to scratch this PR and I do a new one that is doing a small overhaul:

Then release this as new major version (because the error event might be breaking when not registered and so cases that ignored errors silently now would throw an error (again).

Opinions?

foxriver76 commented 7 months ago

Sounds good to me

Apollon77 commented 7 months ago

Grrmppff ... If we want to keep the "new" event then we need to stay at 0.6.1 of file-stream-rotator because of https://github.com/rogerc/file-stream-rotator/pull/107 ...

Apollon77 commented 7 months ago

superceeded by #389