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.

Sign up for GitHub

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

Jump to bottom

Errors swallowed during generateOperationFiles #46

Open
tswaters opened this issue Oct 22, 2020 · 5 comments · May be fixed by #47
Open

Errors swallowed during generateOperationFiles #46

tswaters opened this issue Oct 22, 2020 · 5 comments · May be fixed by #47

Comments

@tswaters
Copy link
Collaborator

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 pushed a commit to tswaters/openapi3-generator that referenced this issue Oct 22, 2020
Remove superfluous resolve
fb54625 
Fixes openapi-contrib#46
@tswaters tswaters linked a pull request Oct 22, 2020 that will close this issue
Open
@tswaters
Copy link
Collaborator Author

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

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

fmvilas commented Sep 22, 2022

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

tswaters commented Feb 7, 2023

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove superfluous resolve tswaters/openapi3-generator
3 participants
@fmvilas @tswaters @caligo-erik