rogerc / file-stream-rotator

NodeJS file stream rotator
MIT License
143 stars 69 forks source link

fix Throw exist error on mkdirSync #81

Closed RyuDoizaki closed 2 years ago

RyuDoizaki commented 3 years ago

When multiple processes are started at the same time, an existed error may occur when creating a folder. Fixed mkdirSync to not throw an error if it was an existed error.

Apollon77 commented 3 years ago

I support this change, also had a crash thats why.

But if we add try/catch then also do one for the fileExists ... I also saw that crashign in special permission scenarios

Means88 commented 2 years ago

May add the option recursive

if (!fs.existsSync(fullPath)) {
  try {
    // supported in Node 10.x, just ignored under version 10 so it's backward compatible
    fs.mkdirSync(fullPath, { recursive: true }); 
  } catch(e) {
    if (e.code !== 'EEXIST') {
      throw e;
    }
  }
}
Apollon77 commented 2 years ago

@Means88 Yes, but then you could/should refactor more because the whole logic of this method is to do that recursive creation ... Additionally this would require to set node-js engine version to be minimum v10.12.0 (which is not a problem, just a fact). But yes in this case there would be no "EEXISTS" error at all :-)

Means88 commented 2 years ago

@Apollon77 Yes, you are right. There should be two versions released IMO. One released in patch version with backward compatibility and the other released in minor version for the breaking change below Node.js 10.12.0 (since the major version is 0)

Apollon77 commented 2 years ago

@Means88 Honestly ... Nodejs 10 is EOL since nearly a year ... so ... ... ... (but yes formally needs to be a major upgrade). SO maybe just adding this try/catch is least invasive

Means88 commented 2 years ago

@Apollon77 It's also OK for me 😄

The most critical problem may be that I found this project haven't been updated since two years ago, and the owner didn't have activities on GitHub last two years...I just create directories manually before I use this package to avoid the error.

image image

Maybe I can infer the downloads of this package is mostly from winston-daily-rotate-file. It's better to deprecate this package and commit changes to winston-daily-rotate-file directly I think...

Apollon77 commented 2 years ago

@rogerc Any chance to accept the PR? Alternatively I could also offer myself as secondary maintainer

rogerc commented 2 years ago

@Apollon77 I've merged the PR. I've made a few other updates. Will push to NPM in the next day or two.

@RyuDoizaki thanks for submitting PR.

Apollon77 commented 2 years ago

@rogerc A big thank you for this!