Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent crash in bad timing cases #366

Closed
wants to merge 2 commits into from

Conversation

Apollon77
Copy link
Collaborator

@Apollon77 Apollon77 commented Jan 5, 2023

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

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)

```
@Apollon77
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@mattberther @wbt Any chance?

@wbt
Copy link
Collaborator

wbt commented Mar 20, 2023

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

@Apollon77
Copy link
Collaborator Author

@mattberther @indexzero Maybe you can help?

@vbasset-repam
Copy link

+1

@Apollon77
Copy link
Collaborator Author

@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
Copy link

image

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

@foxriver76
Copy link
Collaborator

@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
Copy link

DABH commented Aug 17, 2023

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
Copy link
Collaborator Author

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

@wbt
Copy link
Collaborator

wbt commented Aug 17, 2023

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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
Copy link
Member

@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 Apollon77 mentioned this pull request Feb 6, 2024
@Apollon77
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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:

  • es6 (var -> const/let)
  • introduce an error event which is emitted whenever an error happens, also if it is catched and then use this in all places ionstead of "silently ignoring " the exceptions as of now. @DABH Is there anything special to check from Winston/Winston-Transport side for that error event? I did not found anything so far
  • Update deps

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
Copy link
Collaborator

Sounds good to me

@Apollon77
Copy link
Collaborator Author

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

@Apollon77 Apollon77 mentioned this pull request Feb 7, 2024
@Apollon77
Copy link
Collaborator Author

superceeded by #389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants