search

openapi-contrib / openapi3-generator

Use your API OpenAPI 3 definition to generate code, documentation, and literally anything you need.
Apache License 2.0
90 stars 27 forks source link

Errors swallowed during generateOperationFiles #46

Open tswaters opened 4 years ago

tswaters commented 4 years ago

There's a bug here: https://github.com/openapi-contrib/openapi3-generator/blob/0.13.0/lib/generator.js#L144-L147

    Promise.all(
      _.map(files, (operation, operation_name) => generateOperationFile(config, operation, operation_name))
    ).then(resolve).catch(reject);
    resolve();

The outer promise here is being resolved before the Promise.all has a chance to finish, and if there's a rejection, it is ignored. I was running this tool from a docker container and only 75% of the expected files were being generated. I tracked it down to errors on that line not being handled properly..... With the resolve removed, I get the error back properly:

Error: EMFILE: too many open files, open '/api-docs/templates/my-template/$$path$$.js'

Why I'm getting that in docker and not locally is still a mystery to me... but this routine seems inefficient. As as test I logged out the name of the file that was being read here: https://github.com/openapi-contrib/openapi3-generator/blob/0.13.0/lib/generator.js#L96-L97

const generateOperationFile = (config, operation, operation_name) => new Promise((resolve, reject) => {
  console.log(path.join(config.root, config.file_name))
  fs.readFile(path.join(config.root, config.file_name), 'utf8', (err, data) => {

And I get the same file logged out a couple thousands times.... Because it's a Promise.all I imagine node will try to load all of them at the same time, and I'm guessing in my particular case, there's not enough file descriptors available and it blows up.... Further, I logged out the file path that it writes in this routine, and it's writing out the same file multiple times.... Seems like the data structure being passed generated / passed in there could do with a Sets instead to avoid duplicates?

tswaters commented 4 years ago

I've also added a commit to the linked PR to improve performance a bit. For each operation it finds, it's writing out an incomplete file. Fortunately, the last one always has the rest of files generated so it includes everything. In my limited testing, my generation time went from ~6s to ~2s. Also, this will likely fix the too many file descriptors problem.

tswaters commented 2 years ago

@philsturgeon @fmvilas friendly bump.... Just had one of our developers hit this issue. Looking into how to fix the max file descriptors thing on their OS, but it would be great to fix the underlying problem. If there's anything I can do to push this along, please let me know.

fmvilas commented 2 years ago

I'm sorry but I'm not maintaining this library anymore since ~3 years ago. I bet @philsturgeon is also super busy so my advice is that you folks chat to discuss what's the best way forward.

tswaters commented 1 year ago

I just checked npm info openapi3-generator and fmvilas fmvilas@gmail.com is listed as the only maintainer.

If you want, feel free to add me as a maintainer (tyler.waters@gmail.com) -- I can get this PR merged, fixed up the 26 vulns currently reported by npm audit ( 20 when showing only prod deps) -- and get a new version added to npm.

I can appreciate the module hasn't been updated in many years now, and I don't imagine I'll be pumping out features or major versions -- but I'd like to get these things fixed up at least.

caligo-erik commented 1 year ago

I bumped into this one today. Target file already existed and was read-only, and the error was skipped.