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

Restrict parallel execution of load hook. #4200

Merged

Conversation

schummar
Copy link
Contributor

@schummar schummar commented Aug 2, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Follow up to #4170
I had more "EMFILE: too many open files" errors in another project and realized the reason for this that the fix in #4170 does not apply when a load hook is provided. We can assume that load hooks will often use fs.readFile as well and therefore be subject to the same problem with too many parallel file read. I think it makes sense to extend the scope of the readQueue and also channel the load hook through it.

@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #4200 (9e949af) into master (b34a411) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4200      +/-   ##
==========================================
- Coverage   98.37%   98.37%   -0.01%     
==========================================
  Files         202      202              
  Lines        7254     7253       -1     
  Branches     2127     2126       -1     
==========================================
- Hits         7136     7135       -1     
  Misses         58       58              
  Partials       60       60              
Impacted Files Coverage Δ
src/ModuleLoader.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b34a411...9e949af. Read the comment docs.

@schummar
Copy link
Contributor Author

schummar commented Aug 2, 2021

The failed Windows test seems to be a temporary GitHub Issue to me.

Not sure what to do about the -0.01% coverage drop, because all edited lines are fully covered, it's just that's it's one line less than before, bringing the average down. Can this check be ignored? Otherwise I'd have to write a test for something completely unrelated to the PR 😁

@lukastaegert
Copy link
Member

Can this check be ignored?

I know this edge case. Unfortunately, I did not find a way to fix this easily, just ignore it. I will have a look at the PR likely tomorrow.

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.

None yet

2 participants