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

fix: Use new stream rather than pipe #700

Merged
merged 3 commits into from
Jun 24, 2019
Merged

fix: Use new stream rather than pipe #700

merged 3 commits into from
Jun 24, 2019

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented Jun 22, 2019

Fixes #661.

We use a single long-lived stream for all of our logic. If the backend streams drops, we unpipe the old stream and pipe and new stream. After many re-connects, it seems to cause EventEmitter leaks (#661). By using new streams rather than piping/unpiping a long-lived stream, the MaxListenersExceededWarning should no longer appear.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 22, 2019
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thanks! Since this is a seemingly unrelated fix to #661, can you explain what you are doing in the PR description?

dev/src/watch.ts Outdated Show resolved Hide resolved
code = change.cause.code!;
message = change.cause.message!;
}
// @todo: Surface a .code property on the exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may have now earned a Moma badge by moving this TODO around more than 10 times :)

@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #700 into master will decrease coverage by 0.02%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #700      +/-   ##
=========================================
- Coverage   96.23%   96.2%   -0.03%     
=========================================
  Files          20      20              
  Lines        2230    2215      -15     
  Branches      470     470              
=========================================
- Hits         2146    2131      -15     
  Misses         20      20              
  Partials       64      64
Impacted Files Coverage Δ
dev/src/watch.ts 97.57% <92.85%> (-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 09d83e4...5c25a44. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #700 into master will decrease coverage by 0.16%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   96.23%   96.07%   -0.17%     
==========================================
  Files          20       20              
  Lines        2230     2215      -15     
  Branches      470      470              
==========================================
- Hits         2146     2128      -18     
- Misses         20       22       +2     
- Partials       64       65       +1
Impacted Files Coverage Δ
dev/src/watch.ts 96.35% <92.85%> (-1.36%) ⬇️

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 09d83e4...62c922f. Read the comment docs.

@thebrianchen thebrianchen merged commit 0370e03 into master Jun 24, 2019
@thebrianchen thebrianchen deleted the bc/no-leaky branch June 24, 2019 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GRPC-JS: Possible EventEmitter memory leak detected
3 participants