fix: Support more than 100 long-lived streams #623

schmidt-sebastian · 2019-05-15T17:24:02Z

This is a different approach to #614 and solves the same problem. It re-uses more of the original code to achieve the same result. Instead of releasing the client back to the pool when a stream is marked as established, we instead keep it around until the stream is closed.

wilhuff

I like the high level way this works better than the way I did it.

wilhuff · 2019-05-15T17:36:31Z

dev/src/index.ts

@@ -1176,7 +1174,7 @@ export class Firestore {
    let endCalled = false;

    return new Promise((resolve, reject) => {
-      const streamReady = () => {
+      const releaseStream = () => {


I don't like this name. "released" can be confused with releasing back into the pool. I certainly was confused that way when reading this.

This promise represents that the stream is ready for use or initialized or something like that.

I changed the PR to keep the renames of your original commit.

dev/src/index.ts

wilhuff · 2019-05-15T17:40:34Z

dev/src/index.ts

-      });
+        return this._initializeStream(resultStream, requestTag, request);
+      })
+        .then(stream => {


async/await?

I cleaned this up. I also removed the return value from _initializeStream as part of this since we always return the unmodified resultStream.

codecov · 2019-05-15T18:00:52Z

Codecov Report

Merging #623 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
+ Coverage   61.45%   61.47%   +0.02%     
==========================================
  Files          21       21              
  Lines        3409     3411       +2     
  Branches      460      459       -1     
==========================================
+ Hits         2095     2097       +2     
  Misses       1252     1252              
  Partials       62       62

Impacted Files	Coverage Δ
dev/src/pool.ts	`97.14% <ø> (-0.36%)`	⬇️
dev/src/index.ts	`95.14% <100%> (+0.11%)`	⬆️

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 83c5dd9...0ffaee6. Read the comment docs.

codecov · 2019-05-15T18:00:52Z

Codecov Report

Merging #623 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
+ Coverage   61.52%   61.68%   +0.16%     
==========================================
  Files          21       21              
  Lines        3407     3422      +15     
  Branches      460      459       -1     
==========================================
+ Hits         2096     2111      +15     
  Misses       1250     1250              
  Partials       61       61

Impacted Files	Coverage Δ
dev/src/index.ts	`95.19% <100%> (+0.17%)`	⬆️
dev/src/util.ts	`100% <100%> (ø)`	⬆️
dev/src/pool.ts	`97.36% <100%> (-0.14%)`	⬇️

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 43ac9c6...29caec6. Read the comment docs.

wilhuff · 2019-05-15T18:56:02Z

dev/src/index.ts

+        });
+
+        const resultStream = bun([stream, logStream]);
+        resultStream.on('close', lifetime.resolve);


Are streams guaranteed to emit the close event? What happens in the case of an error?

According to https://nodejs.org/api/stream.html, yes:

"A Writable stream will always emit the 'close' event if it is created with the emitClose option."
"A Readable stream will always emit the 'close' event if it is created with the emitClose option."

emitClose defaults to true.

I originally trusted this, but I spent more time and added test asserts. It turns out that the close event is not always emitted. To make the unit and system tests pass, I also have to wait for error/end and finish on writeable streams.

wilhuff · 2019-05-15T18:58:22Z

dev/system-test/firestore.ts

+      await Promise.all(emptyResults.map(d => d.promise));
+      ref.set({i: 1337});
+      await Promise.all(documentResults.map(d => d.promise));
+      unsubscribeCallbacks.forEach(c => c());


This test verifies that all 150 listeners succeed but doesn't verify that everything has been properly released to the pool.

Is it possible to check that pool.size is 150 once the listeners are started and then get back to zero after?

I worry that this test can succeed even if you remove the line that resolves the lifetime promise.

I added a "shutdown" block to each tests that verifies that the operation count goes back to zero. I had to change some of the unit tests to make this work.

wilhuff · 2019-05-15T22:03:00Z

This is now failing tests under node 8:

  1) Query watch
       re-opens on unexpected stream end:
      Expected message of type 'end' but got 'open' with '{}'.
      + expected - actual
      -open
      +end
      at DeferredListener.await (build/test/watch.js:143:41)
      at StreamHelper.await (build/test/watch.js:186:43)
      at watchHelper.await.then (build/test/watch.js:604:41)
      at <anonymous>
  2) Query watch
       "after each" hook for "re-opens on unexpected stream end":
      AssertionError: expected 1 to equal 0
      + expected - actual
      -1
      +0
      at Object.verifyInstance (build/test/util/helpers.js:87:56)
      at Context.afterEach (build/test/watch.js:544:19)

LGTM otherwise.

wilhuff

LGTM. Awesome!

…/nodejs-firestore into mrschmidt-streamclose

schmidt-sebastian · 2019-05-16T03:57:17Z

The test failure for Node 8 was due to a flake, which I fixed.

jakeleventhal · 2019-05-28T02:36:40Z

@wilhuff @schmidt-sebastian I just upgraded to @google-cloud/firestore@2.0.0 and since upgrading, I've been getting all of these warnings that say:
(node:2870) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 connectivityStateChanged listeners added. Use emitter.setMaxListeners() to increase limit

I read through the release notes and found that this thread is probably the reason that I am seeing these warnings. I tried downgrading back to v1.3.0 and no longer saw the messages in my logs. I believe that it makes sense to take care of the warning that I'm seeing within the @google-cloud/firestore package itself, as the user doesn't really have a good way to know when the listeners will be added.

What are your guys' thoughts?

schmidt-sebastian · 2019-05-28T02:48:23Z

@jakeleventhal Thanks for letting us know! I think I have seen this warning once before as well, so it may indeed be related to this change (which adds a lot of listeners). I filed #661 to track this.

Revert a1b74a41

bdd3ce4

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 15, 2019

Don't release clients before stream close

4d5250a

schmidt-sebastian force-pushed the mrschmidt-streamclose branch from f8404db to 4d5250a Compare May 15, 2019 17:25

schmidt-sebastian requested a review from wilhuff May 15, 2019 17:39

wilhuff reviewed May 15, 2019

View reviewed changes

Address feedback

0ffaee6

schmidt-sebastian assigned wilhuff May 15, 2019

wilhuff reviewed May 15, 2019

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff May 15, 2019

schmidt-sebastian added 2 commits May 15, 2019 14:45

Verify stream count

52ff6d3

Remove empty line

18c1a0e

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian May 15, 2019

JustinBeckwith changed the title ~~Fix: Support more than 100 long-lived streams~~ fix: Support more than 100 long-lived streams May 15, 2019

wilhuff removed their assignment May 15, 2019

Merge branch 'master' into mrschmidt-streamclose

1c4d016

wilhuff approved these changes May 16, 2019

View reviewed changes

schmidt-sebastian added 4 commits May 15, 2019 20:49

Fix flaky test

cc8d75d

Moving Deferred Promise to src/util

75fc446

Merge branch 'mrschmidt-streamclose' of https://github.com/googleapis…

3a529b1

…/nodejs-firestore into mrschmidt-streamclose

Fix typo

29caec6

schmidt-sebastian merged commit 9474e3f into master May 16, 2019

schmidt-sebastian deleted the mrschmidt-streamclose branch May 17, 2019 22:56

schmidt-sebastian mentioned this pull request May 28, 2019

GRPC-JS: Possible EventEmitter memory leak detected #661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Support more than 100 long-lived streams #623

fix: Support more than 100 long-lived streams #623

schmidt-sebastian commented May 15, 2019

wilhuff left a comment

wilhuff May 15, 2019

schmidt-sebastian May 15, 2019

wilhuff May 15, 2019

schmidt-sebastian May 15, 2019

codecov bot commented May 15, 2019

codecov bot commented May 15, 2019 •

edited

wilhuff May 15, 2019

schmidt-sebastian May 15, 2019

wilhuff May 15, 2019

wilhuff May 15, 2019

schmidt-sebastian May 15, 2019

wilhuff commented May 15, 2019 •

edited

wilhuff left a comment

schmidt-sebastian commented May 16, 2019

jakeleventhal commented May 28, 2019

schmidt-sebastian commented May 28, 2019 •

edited

fix: Support more than 100 long-lived streams #623

fix: Support more than 100 long-lived streams #623

Conversation

schmidt-sebastian commented May 15, 2019

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 15, 2019

Codecov Report

codecov bot commented May 15, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff commented May 15, 2019 • edited

wilhuff left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented May 16, 2019

jakeleventhal commented May 28, 2019

schmidt-sebastian commented May 28, 2019 • edited

codecov bot commented May 15, 2019 •

edited

wilhuff commented May 15, 2019 •

edited

schmidt-sebastian commented May 28, 2019 •

edited