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

benchmark: Repair the fs/readfile benchmark #7818

Closed
wants to merge 1 commit into from

Conversation

sorinh
Copy link

@sorinh sorinh commented Jul 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

The readfile benchmark of fs type was throwing an exception because the benchmark tried to read the file after it got unlinked at the end of the run.

Signed-off-by: Sorin Baltateanu sorin.baltateanu@intel.com

Signed-off-by: Sorin Baltateanu <sorin.baltateanu@intel.com>
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jul 21, 2016
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jul 21, 2016
@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

I'm unable to recreate the exception on macos and I'm not sure if introducing a timer here is the best approach. Pinging @mscdex for his thoughts...

@AndreasMadsen
Copy link
Member

I remember having seen that error too, but it should be fixed after 8bb59fd landed. Though this fix may be better.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stil seems to make sense to me.

CI: https://ci.nodejs.org/job/node-test-commit/8078/

jasnell pushed a commit that referenced this pull request Mar 24, 2017
PR-URL: #7818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

Landed in d9b0e4c

@jasnell jasnell closed this Mar 24, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 25, 2017

I'm confused as to why this was merged when there was already a process.exit(0); added much earlier?

MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
PR-URL: #7818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants