Skip to content

Conversation

@FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Feb 6, 2025

Previously we would remove event listeners when calling destroy. This meant that consumers of our streams would not be notified that the stream was closed causing issues like Cannot call write after a stream was destroyed.

Instead, we want consumers to be notified when a stream is torn down. One issue however is that it introduces new issues where some code paths throw Premature close. This seems to be an underlying problem in the MetaMask stream architecture and the error message is ignored on the extension client. We do the same here.

This will eventually also allow us to remove closeConnections from the SnapController constructor.

@FrederikBolding FrederikBolding force-pushed the fb/fix-stream-destroy branch 2 times, most recently from e850acc to 42e9cfc Compare February 6, 2025 12:38
@codecov
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.29%. Comparing base (6cea1bf) to head (87a111b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...cution-environments/src/common/BaseSnapExecutor.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3074      +/-   ##
==========================================
+ Coverage   98.27%   98.29%   +0.02%     
==========================================
  Files         417      417              
  Lines       11759    11759              
  Branches     1827     1827              
==========================================
+ Hits        11556    11559       +3     
+ Misses        203      200       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@FrederikBolding FrederikBolding marked this pull request as ready for review August 5, 2025 11:42
@FrederikBolding FrederikBolding requested a review from a team as a code owner August 5, 2025 11:42
try {
!stream.destroyed && stream.destroy();
stream.removeAllListeners();
stream.destroy();
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should still check !stream.destroyed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I had found evidence that you could call it multiple times without issue, but I can't find it now. Will just re-add the check

@FrederikBolding FrederikBolding added this pull request to the merge queue Aug 5, 2025
Merged via the queue into main with commit 5c21b34 Aug 5, 2025
119 of 120 checks passed
@FrederikBolding FrederikBolding deleted the fb/fix-stream-destroy branch August 5, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants