domain: error handler runs outside of its domain #26211

misterdjules · 2019-02-20T01:20:36Z

Before this change, domains' error handlers would run with the
corresponding domain as the active domain. This creates the
possibility for domains' error handlers to call themselves recursively
if an event emitter created in the error handler emits an error, or if
the error handler throws an error.

This change sets the active domain to be the domain's parent (or null
if the domain for which the error handler is called has no parent) to
prevent that from happening.

Fixes: #26086

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

Documentation not added yet, I'd like to discuss the changes first and whether folks would be open to that before digging into that.

@nodejs/domains Thoughts?

Before this change, domains' error handlers would run with the corresponding domain as the active domain. This creates the possibility for domains' error handlers to call themselves recursively if an event emitter created in the error handler emits an error, or if the error handler throws an error. This change sets the active domain to be the domain's parent (or null if the domain for which the error handler is called has no parent) to prevent that from happening. Fixes: nodejs#26086

nodejs-github-bot · 2019-02-20T01:20:38Z

@misterdjules build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2669/pipeline
CI: https://ci.nodejs.org/job/node-test-pull-request/23851/

addaleax · 2019-02-20T10:39:43Z

Is this semver-major? I’m not sure whether this is the kind of change we’d still want to apply to domains, after we’ve basically left it alone for years?

BridgeAR · 2019-02-20T15:06:30Z

@misterdjules what about using a symbol property to detect the recursion if the exception is thrown by the same domain? I believe that would also solve this problem and it seems less of a breaking change to me.

I do not have a strong opinion about improving domain even though it's deprecated but I guess there is no big downside either (especially as people still use it).

lib/domain.js

misterdjules · 2019-02-20T17:40:17Z

@BridgeAR

what about using a symbol property to detect the recursion if the exception is thrown by the same domain? I believe that would also solve this problem and it seems less of a breaking change to me.

That sounds interesting! Do you mind elaborating a bit more on that to make sure I understand the details of what you're suggesting? Maybe with a diff of the change you'd suggest?

misterdjules · 2019-02-20T17:46:10Z

@addaleax

Is this semver-major?

I think it's probably wise to make this semver-major. Thanks @BridgeAR for adding the label!

I’m not sure whether this is the kind of change we’d still want to apply to domains, after we’ve basically left it alone for years?

I'd say if folks think it's a good change, let's merge it. Otherwise this is exactly the use cases for which I'd like to build consensus around supporting userland domain implementations, like with #24279.

BridgeAR · 2019-02-21T12:34:53Z

@misterdjules I tried it and it seems trickier than I first anticipated. I am going to have another look at it soon.

misterdjules · 2019-02-26T23:25:06Z

@BridgeAR

I tried it and it seems trickier than I first anticipated. I am going to have another look at it soon.

Did you have time to take a look at that?

BridgeAR · 2019-03-01T00:37:56Z

@misterdjules no, sorry, not yet. Please do not wait on it either. As long as I can not come up with something better, it does seem reasonable. But to give a LG I'll have to dig deeper.

BridgeAR · 2019-03-27T16:51:12Z

@nodejs/domains @nodejs/tsc this could use some reviews.

mcollina

LGTM, this makes sense even if it makes me uncomfortable in making semver-major changes to something we have deprecated long ago.

BridgeAR

LGTM. The changed behavior is definitely better than the current one and as long as we can not come up with an alternative solution, this seems a good idea.

BridgeAR · 2019-04-03T02:51:35Z

@nodejs/tsc PTAL. This requires one more LG from a TSC member.

nodejs-github-bot · 2019-04-03T02:52:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22151/

BridgeAR · 2019-04-03T02:52:45Z

@misterdjules it would be nice if you would fix my comments. As son as that's done I'll add the author ready label.

Co-Authored-By: misterdjules <jgilli@fastmail.fm>

misterdjules · 2019-04-03T16:29:03Z

@BridgeAR Done! Thank you so much for following-up on this ❤️

nodejs-github-bot · 2019-04-03T16:37:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22173/

nodejs-github-bot · 2019-04-04T13:03:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22188/

gireeshpunathil · 2019-04-26T04:19:45Z

@misterdjules - something like this:

let txn = false;

d.on('error', (er) => {
  console.log(er)
  try {
    server.close();
  } catch (er2) {
    // ignore error
  }
  if (txn === true)
    d.enter();
    d.run();
});
d.run(() => {
  txn = true;
  server = http.createServer((req, res) => {
    // server code
  });
  server.listen(PORT);
  server.on('close', () => {
    txn = false;
  })
})

where under some conditions (where we know re-entrance is safe) the err'd domain can be re-entered.

misterdjules · 2019-04-30T21:06:30Z

@gireeshpunathil

I don't see a reason why one couldn't manually enter any domain in that domain's error handler. I'd be curious to understand what the use case for this would be though.

When a domain's error handler is called as a result of an error event that is emitted synchronously, this PR makes sure that the actual domain is restored once the error event handler's execution completed.

When a domain's error handler is called as a result of an error being thrown, the stack is unwound and I don't think there is a use case for resuming the execution of the code after the domain error handler completed in the context of the same domain.

If the use case is about running the actual domain error handler with that domain being active, then I'd be curious to hear more about that too since this PR specifically prevents that from happening.

Also, I don't think the code you provided above actually runs, it might be easier to answer your question(s) if you could provide me with a running repro of the issues you have in mind.

Thanks for the questions though, very much appreciated 👍

mcollina · 2019-05-01T00:46:44Z

This can now land. @nodejs/tsc I propose we land this in v12.0.0, as it has been more or less our slowness that it did not make the cut. If we want to land this, it’s better to land it now instead of waiting for another full LTS cycle.

gireeshpunathil · 2019-05-01T17:02:05Z

@misterdjules - thanks for the detailed explanation, no - I don't have any use case that specifically requires the domain being active while its handler runs, so my question is hypothetical.

Trott · 2019-05-02T06:14:38Z

@nodejs/tsc I propose we land this in v12.0.0,

Unless there's something I'm missing (which there usually is), that's up to @nodejs/release. (Determining the content of all releases is delegated to @nodejs/release in their charter/mandate. TSC maintains authority over what lands in the master branch. Step one is no doubt to get this landed on master.)

Not that it matters much, since I'm not on the Release WG, but I'm neutral on landing this in 12.x.x. Seems fine to me, but I'm also fine if Release WG decides not to do that.

nodejs-github-bot · 2019-05-19T20:33:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23207/

addaleax · 2019-05-19T21:34:29Z

I think this is good to go apart from the linter failure, but that could also be fixed while landing?

misterdjules · 2019-05-20T18:17:40Z

@addaleax Added a commit that should fix the linting error, thanks for bringing that up!

nodejs-github-bot · 2019-05-20T18:19:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23235/

nodejs-github-bot · 2019-05-26T15:30:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23350/

Trott · 2019-06-13T03:43:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23851/

Before this change, domains' error handlers would run with the corresponding domain as the active domain. This creates the possibility for domains' error handlers to call themselves recursively if an event emitter created in the error handler emits an error, or if the error handler throws an error. This change sets the active domain to be the domain's parent (or null if the domain for which the error handler is called has no parent) to prevent that from happening. Fixes: nodejs#26086 PR-URL: nodejs#26211 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2019-06-13T05:03:23Z

Landed in 43a5170

Forgot a CITGM. Sorry. CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1872/

misterdjules · 2019-06-13T17:49:59Z

@Trott Thanks for merging this 🙏 It seems that most (if not all) CITGM Jenkins job failed with the following error:

22:04:40 + git rebase --committer-date-is-author-date origin/master
22:04:42 First, rewinding head to replay your work on top of it...
22:04:44 Applying: domain: error handler runs outside of its domain
22:04:44 Using index info to reconstruct a base tree...
22:04:44 M	lib/domain.js
22:04:44 M	test/parallel/test-domain-top-level-error-handler-clears-stack.js
22:04:44 M	test/parallel/test-timers-reset-process-domain-on-throw.js
22:04:44 Falling back to patching base and 3-way merge...
22:04:44 CONFLICT (add/add): Merge conflict in test/parallel/test-domain-emit-error-handler-stack.js
22:04:44 Auto-merging test/parallel/test-domain-emit-error-handler-stack.js
22:04:44 Auto-merging lib/domain.js
22:04:44 CONFLICT (content): Merge conflict in lib/domain.js
22:04:44 error: Failed to merge in the changes.
22:04:44 hint: Use 'git am --show-current-patch' to see the failed patch
22:04:44 Patch failed at 0001 domain: error handler runs outside of its domain
22:04:44 Resolve all conflicts manually, mark them as resolved with
22:04:44 "git add/rm <conflicted_files>", then run "git rebase --continue".
22:04:44 You can instead skip this commit: run "git rebase --skip".
22:04:44 To abort and get back to the state before "git rebase", run "git rebase --abort".
22:04:44 Build step 'Conditional steps (multiple)' marked build as failure

Do you want me to look into this?

Also re: landing this in 12.x, I'm happy to open a PR against the v12.x branch if there's no objection to land it in 12.x. /cc @mcollina

Trott · 2019-06-13T17:53:48Z

@Trott Thanks for merging this 🙏 It seems that most (if not all) CITGM Jenkins job failed with the following error:

22:04:40 + git rebase --committer-date-is-author-date origin/master
22:04:42 First, rewinding head to replay your work on top of it...
22:04:44 Applying: domain: error handler runs outside of its domain
22:04:44 Using index info to reconstruct a base tree...
22:04:44 M	lib/domain.js
22:04:44 M	test/parallel/test-domain-top-level-error-handler-clears-stack.js
22:04:44 M	test/parallel/test-timers-reset-process-domain-on-throw.js
22:04:44 Falling back to patching base and 3-way merge...
22:04:44 CONFLICT (add/add): Merge conflict in test/parallel/test-domain-emit-error-handler-stack.js
22:04:44 Auto-merging test/parallel/test-domain-emit-error-handler-stack.js
22:04:44 Auto-merging lib/domain.js
22:04:44 CONFLICT (content): Merge conflict in lib/domain.js
22:04:44 error: Failed to merge in the changes.
22:04:44 hint: Use 'git am --show-current-patch' to see the failed patch
22:04:44 Patch failed at 0001 domain: error handler runs outside of its domain
22:04:44 Resolve all conflicts manually, mark them as resolved with
22:04:44 "git add/rm <conflicted_files>", then run "git rebase --continue".
22:04:44 You can instead skip this commit: run "git rebase --skip".
22:04:44 To abort and get back to the state before "git rebase", run "git rebase --abort".
22:04:44 Build step 'Conditional steps (multiple)' marked build as failure

Do you want me to look into this?

Also re: landing this in 12.x, I'm happy to open a PR against the v12.x branch if there's no objection to land it in 12.x. /cc @mcollina

Oh, right, because I set the parameters as if the PR hadn't yet landed. Let me try again:

CITGM against master (which is currently at 4b1bcae and includes this PR): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1874/

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

nodejs-github-bot added the domain Issues and PRs related to the domain subsystem. label Feb 20, 2019

misterdjules mentioned this pull request Feb 20, 2019

Domain error handler executed in context of errored domain. #26086

Closed

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 20, 2019

BridgeAR reviewed Feb 20, 2019

View reviewed changes

lib/domain.js Outdated Show resolved Hide resolved

lib/domain.js Outdated Show resolved Hide resolved

vdeturckheim approved these changes Feb 20, 2019

View reviewed changes

misterdjules mentioned this pull request Feb 20, 2019

When using handleUncaughtExceptions, if an error is thrown within a stream callback in the uncaughtException handler, restify enters an infinite loop. restify/node-restify#1749

Open

3 tasks

misterdjules mentioned this pull request Mar 14, 2019

fix: properly handle non-errors thrown in domains restify/node-restify#1757

Merged

3 tasks

mcollina approved these changes Mar 27, 2019

View reviewed changes

BridgeAR approved these changes Apr 3, 2019

View reviewed changes

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 3, 2019

Apply suggestions from code review

d90a46c

Co-Authored-By: misterdjules <jgilli@fastmail.fm>

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2019

This was referenced Apr 8, 2019

Node.js Foundation Technical Steering Committee (TSC) Meeting 2019-04-10 nodejs/TSC#688

Closed

Node.js Foundation Technical Steering Committee (TSC) Meeting 2019-04-10 mhdawson/temp-testing#35

Closed

BridgeAR added the review wanted PRs that need reviews. label Apr 15, 2019

Trott approved these changes Apr 29, 2019

View reviewed changes

Trott removed the review wanted PRs that need reviews. label Apr 29, 2019

fix lint error

386845d

Trott closed this Jun 13, 2019

addaleax mentioned this pull request Jun 20, 2019

parallel/test-domain-error-types can crash if GC timings get unlucky #28275

Closed

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

domain: error handler runs outside of its domain #26211

domain: error handler runs outside of its domain #26211

misterdjules commented Feb 20, 2019

nodejs-github-bot commented Feb 20, 2019 •

edited

Loading

addaleax commented Feb 20, 2019

BridgeAR commented Feb 20, 2019

misterdjules commented Feb 20, 2019

misterdjules commented Feb 20, 2019

BridgeAR commented Feb 21, 2019

misterdjules commented Feb 26, 2019

BridgeAR commented Mar 1, 2019

BridgeAR commented Mar 27, 2019

mcollina left a comment

BridgeAR left a comment

BridgeAR commented Apr 3, 2019

nodejs-github-bot commented Apr 3, 2019

BridgeAR commented Apr 3, 2019

misterdjules commented Apr 3, 2019

nodejs-github-bot commented Apr 3, 2019

nodejs-github-bot commented Apr 4, 2019

gireeshpunathil commented Apr 26, 2019

misterdjules commented Apr 30, 2019

mcollina commented May 1, 2019

gireeshpunathil commented May 1, 2019

Trott commented May 2, 2019

nodejs-github-bot commented May 19, 2019

addaleax commented May 19, 2019

misterdjules commented May 20, 2019

nodejs-github-bot commented May 20, 2019

nodejs-github-bot commented May 26, 2019

Trott commented Jun 13, 2019

Trott commented Jun 13, 2019

misterdjules commented Jun 13, 2019

Trott commented Jun 13, 2019

domain: error handler runs outside of its domain #26211

domain: error handler runs outside of its domain #26211

Conversation

misterdjules commented Feb 20, 2019

Checklist

nodejs-github-bot commented Feb 20, 2019 • edited Loading

addaleax commented Feb 20, 2019

BridgeAR commented Feb 20, 2019

misterdjules commented Feb 20, 2019

misterdjules commented Feb 20, 2019

BridgeAR commented Feb 21, 2019

misterdjules commented Feb 26, 2019

BridgeAR commented Mar 1, 2019

BridgeAR commented Mar 27, 2019

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Apr 3, 2019

nodejs-github-bot commented Apr 3, 2019

BridgeAR commented Apr 3, 2019

misterdjules commented Apr 3, 2019

nodejs-github-bot commented Apr 3, 2019

nodejs-github-bot commented Apr 4, 2019

gireeshpunathil commented Apr 26, 2019

misterdjules commented Apr 30, 2019

mcollina commented May 1, 2019

gireeshpunathil commented May 1, 2019

Trott commented May 2, 2019

nodejs-github-bot commented May 19, 2019

addaleax commented May 19, 2019

misterdjules commented May 20, 2019

nodejs-github-bot commented May 20, 2019

nodejs-github-bot commented May 26, 2019

Trott commented Jun 13, 2019

Trott commented Jun 13, 2019

misterdjules commented Jun 13, 2019

Trott commented Jun 13, 2019

nodejs-github-bot commented Feb 20, 2019 •

edited

Loading