Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

ttd: fixing ttd hooks in node_file #461

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

MSLaguana
Copy link
Contributor

Some refactoring had caused TTD to fail whenever node::fs::AfterInteger
was called for a reason other than node::fs::Read.

Also moving the TTD component outside of the after.Proceed check to make sure that we always report the end of the period where libuv would be writing to the buffer.

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

ttd

Copy link
Contributor

@mike-kaufman mike-kaufman left a comment

Choose a reason for hiding this comment

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

lgtm

@kfarnung
Copy link
Contributor

kfarnung commented Feb 7, 2018

nit: In this case the "subsystem" is actually "fs" or more broadly "src." since "ttd" isn't really a subsystem that's being modified here.

EDIT: I tend to use this as my defacto list of subsystems: https://github.com/nodejs/core-validate-commit/blob/master/lib/rules/subsystem.js

MSLaguana added a commit to MSLaguana/node-chakracore that referenced this pull request Feb 9, 2018
Some refactoring had caused TTD to fail whenever node::fs::AfterInteger
was called for a reason other than node::fs::Read.

PR-URL: nodejs#461
Reviewed-By: Mike Kaufman <mike.kaufman@microsoft.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Some refactoring had caused TTD to fail whenever node::fs::AfterInteger
was called for a reason other than node::fs::Read.

PR-URL: nodejs#461
Reviewed-By: Mike Kaufman <mike.kaufman@microsoft.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
@MSLaguana MSLaguana merged commit f162a30 into nodejs:master Feb 9, 2018
@MSLaguana MSLaguana deleted the fixFsReadTTD branch February 9, 2018 04:37
@MSLaguana
Copy link
Contributor Author

Thanks everyone!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants