Memory leak fixes - stream and filter life cycles #2070

kumavis · 2017-09-12T05:45:11Z

filters:

filters are tied to the life cycle of their provider-engine/json-rpc-engine
previous provider-engine never died so it kept everything
now we create a mini json-rpc-engine for each page that does the filters, then passed on to our main stack
that mini json-rpc-engine dies when the page closes

streams:

we now clean up streams after the page disconnects

…filter-leak-fix

kumavis · 2017-09-12T05:47:04Z

This will likely need #2069 to be merged in before tests will pass (seems we need newer versions of chrome/firefox?)

kumavis · 2017-09-12T05:57:15Z

I think the browser support issue is from eth-json-rpc-filters es7

…filter-leak-fix3

danfinlay

This is honestly a very hard PR to review. I caught some linty issues, but nothing worth blocking over. I pulled the branch down, it continues to work. It would be cool to have some explanation of the exact leaks it fixes (I know there are filter leaks at several points in the lifecycle, but at least explaining here on the PR which spot this cleans up would be helpful).

Anyways, it works, doesn't break anything, cleans some things up, claims to fix a leaky realm of code, so I'm in favor of it, you can make changes I requested or merge as you like.

danfinlay · 2017-09-13T04:21:44Z

app/scripts/contentscript.js

+function logStreamDisconnectWarning (remoteLabel, err) {
+  let warningMsg = `MetamaskContentscript - lost connection to ${remoteLabel}`
+  if (err) warningMsg += '\n' + err.stack
+  console.warn(warningMsg)


Prefer log.warn() for loglevel support.

danfinlay · 2017-09-13T04:26:56Z

app/scripts/lib/inpage-provider.js

    connectionStream,
-    multiStream,
+    mux,


I like that you've made the naming more uniform across files.

danfinlay · 2017-09-13T04:43:21Z

app/scripts/metamask-controller.js

-      outStream
+      outStream,
+      (err) => {
+        if (err) console.error(err)


Prefer log.error() for loglevel support.

danfinlay · 2017-09-13T04:43:32Z

app/scripts/metamask-controller.js

+    function loggerMiddleware (req, res, next, end) {
+      next((cb) => {
+        if (res.error) {
+          console.error('Error in RPC response:\n', res)


Prefer log.error(), although I wouldn't block over this, as it was already a console.error.

kumavis · 2017-09-13T17:26:08Z

the global log is only available in the background, not inpage and contentscript

kumavis · 2017-09-13T18:16:14Z

breakout originMiddleware, loggerMiddleware, createProviderMiddleware to reduce MetamaskController file size
disconnect filter-blockTracker handler on stream teardown

kumavis · 2017-09-13T22:20:52Z

@FlySwatter updated!

…sion into filter-leak-fix3

…y match 'Premature close' (#26336) ## **Description** We have long seen many errors in Sentry that have the message "Premature close". This PR addresses those that occur when the UI is closed, and when a connected dapp is closed. This error is thrown from within the readable-stream module when multiple streams are passed to the same pipeline. We do not fully understand the issue yet, but it is suspected that we are incorrectly triggering or handling the destroy or end of a stream. Some relevant findings: - others have experienced similar problems: mafintosh/pump#25 - there are two separate "premature close" errors. One pre-existed this PR (and v12.0.0) #24533, and one was introduced with that PR - the one that was introduced with that PR goes away if the readable-stream dependency of object-multiplex is revert back to v2.3.3 - "so I think the problem is that we are explicitly calling destroy on some of the stream's in the pipeline, before the pipeline is finished doing whatever it needs to do when when the streams are ended/closed/finished" - if we use `connectionStream.pipe(mux).pipe(connectionStream);` instead of `pipeline(connectionStream, mux, connectionStream`, the error goes away, but that was changed almost 7 years ago in a PR titled "Memory leak fixes - stream and filter life cycles" #2070 - With reference to `setupMultiplex`: - if I change that line to pipeline(connectionStream, mux, (err) => {, the error goes away - but if I change it to pipeline(mux, connectionStream, (err) => {, the error does not go away We understand enough at this point to know that these error are not affecting user experience. For the moment, we are going to explicitly stop logging them, as the are clogging sentry, and [we can revisit the search for root cause in the future ](#26337) Note that there is at least one outstanding case of a "Premature close" error that can occur when the application is loading. This PR does not attempt to remove that error from being logged. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26336?quickstart=1) ## **Related issues** Fixes: #26284 ## **Manual testing steps** 1. Open the UI 2. Clear the background console 3. Close the UI 4. There should be no "Premature close" error ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

kumavis added 11 commits August 24, 2017 15:44

inpage - use json-rpc-engine for inpage-provider

cc56d0d

inpage - add idRemapMiddleware

440a42b

streams - use pump and published obj-multiplex

57e4805

inpage - distinguish pump vs pipe

0e8e655

metamask - add jsonrpc filter middleware on per-connection engine

9d4c02e

deps - bump jsonrpc filters for log filter formate fix

f5d0a0b

lint - remove dead code

7040162

Merge branch 'master' of github.com:MetaMask/metamask-extension into …

671dafe

…filter-leak-fix

inpage - use obj-multiplex module

ef3bf81

Merge branch 'master' of github.com:MetaMask/metamask-extension into …

41164f6

…filter-leak-fix

contentscript - fix obj-multiplex instantiation and use pump for streams

8545453

kumavis mentioned this pull request Sep 12, 2017

Memory leak fixes - stream and filter life cycles #2034

Closed

Merge branch 'master' of github.com:MetaMask/metamask-extension into …

114dae5

…filter-leak-fix3

2-am-zzz mentioned this pull request Sep 12, 2017

Filters installed by closed dapps persist #543

Closed

danfinlay approved these changes Sep 13, 2017

View reviewed changes

metamask cont - standardize multiplex stream naming

a265144

debug - prefer logger over console

96d1175

kumavis added 2 commits September 13, 2017 15:17

metamask controller - move middleware into seperate files

245c0f0

metamask controller - destroy filter polyfill on disconnect

765ef64

kumavis and others added 4 commits September 13, 2017 15:26

changelog - add note of filter memory leak fix

6ba448e

Merge branch 'master' into filter-leak-fix3

c634ca3

createOriginMiddleware - fix var name

d7097db

Merge branch 'filter-leak-fix3' of github.com:MetaMask/metamask-exten…

0687c82

…sion into filter-leak-fix3

danfinlay merged commit 693655e into master Sep 15, 2017

danfinlay deleted the filter-leak-fix3 branch September 15, 2017 02:40

gghidini mentioned this pull request Sep 30, 2017

Watching events: The request took too long to process #2114

Closed

This was referenced Aug 2, 2024

fix: Stop logging pipeline stream errors in the service worker if they match 'Premature close' #26336

Merged

Identify the root cause of the "Premature close" error #26337

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leak fixes - stream and filter life cycles #2070

Memory leak fixes - stream and filter life cycles #2070

kumavis commented Sep 12, 2017 •

edited

Loading

kumavis commented Sep 12, 2017

kumavis commented Sep 12, 2017

danfinlay left a comment

danfinlay Sep 13, 2017

danfinlay Sep 13, 2017

danfinlay Sep 13, 2017

danfinlay Sep 13, 2017

kumavis commented Sep 13, 2017

kumavis commented Sep 13, 2017 •

edited

Loading

kumavis commented Sep 13, 2017

Memory leak fixes - stream and filter life cycles #2070

Memory leak fixes - stream and filter life cycles #2070

Conversation

kumavis commented Sep 12, 2017 • edited Loading

kumavis commented Sep 12, 2017

kumavis commented Sep 12, 2017

danfinlay left a comment

Choose a reason for hiding this comment

danfinlay Sep 13, 2017

Choose a reason for hiding this comment

danfinlay Sep 13, 2017

Choose a reason for hiding this comment

danfinlay Sep 13, 2017

Choose a reason for hiding this comment

danfinlay Sep 13, 2017

Choose a reason for hiding this comment

kumavis commented Sep 13, 2017

kumavis commented Sep 13, 2017 • edited Loading

kumavis commented Sep 13, 2017

kumavis commented Sep 12, 2017 •

edited

Loading

kumavis commented Sep 13, 2017 •

edited

Loading