-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
"premature close" error when calling destroy on source stream #25
Comments
see also: mafintosh/end-of-stream#14 |
@zxqfox Looked into the issue you referenced above. Noticed that it was for What I am noticing is that when calling Still trying to figure out why |
Found that the fs https://github.com/nodejs/node/blob/v8.9.3/lib/fs.js#L2057 |
…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** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **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.
…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** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **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.
…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** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **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.
I am using node js 8.* and it has new method
destroy
for Readable streams.https://nodejs.org/docs/latest-v8.x/api/stream.html#stream_readable_destroy_error
The following is generating
premature close
error coming fromend-of-stream
module.Any ideas?
The text was updated successfully, but these errors were encountered: