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.

Sign up for GitHub

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

Jump to bottom

fix: ensuring close event is emited after stream has ended. #332

Merged
merged 1 commit into from
Oct 27, 2022
Merged

fix: ensuring close event is emited after stream has ended. #332

merged 1 commit into from
Oct 27, 2022

Conversation

webark
Copy link
Contributor

@webark webark commented Oct 6, 2022

For node 18 compatibility

Fixes #321

Node 18 pipelines specifically require the end event to be emitted and handled before the close event is.

I tried a test with a local tar file and it was still passing without the fix, but a remote tar file failed, so just stuck to that.

@webark webark requested a review from a team as a code owner October 6, 2022 00:33
webark
webark commented Oct 6, 2022
View reviewed changes
lib/parse.js Outdated Show resolved Hide resolved
@webark
Copy link
Contributor Author

webark commented Oct 6, 2022

@jeferson-sb @lukekarrys let me know if you have any thoughts on this.

@webark webark force-pushed the fix/properly-emitting-close-event branch from dd46841 to 096dc4d Compare October 13, 2022 06:10
@webark
Copy link
Contributor Author

webark commented Oct 13, 2022

@lukekarrys squashed the commits and reworded to pass the commit lint.

@trygve-lie trygve-lie mentioned this pull request Oct 20, 2022
Closed
@webark
Copy link
Contributor Author

webark commented Oct 20, 2022

@lukekarrys pinging again to see if you can approve the verifications.

@webark webark mentioned this pull request Oct 25, 2022
Closed
@lukekarrys lukekarrys merged commit 57493ee into isaacs:main Oct 27, 2022
@lukekarrys
Copy link
Contributor

Thanks for sticking with this @webark! I left a note above about a comment, but I decided I'd rather approve and merge and not risk forgetting about this (yet again).

Thanks again! This should be released as a patch within the next week.

@github-actions github-actions bot mentioned this pull request Oct 27, 2022
Merged
@webark webark deleted the fix/properly-emitting-close-event branch October 27, 2022 01:48
@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

Thank you! :) Just saw your previous comment and was about to push a change. I'll check back in a couple of months once node 18 has baked a bit and look into opening a PR to switch it to nextTick

@bergundy
Copy link

nit: Promise.resolve().then which works on Node 10 is equivalent to queueMicrotask and cheaper than nextTick.
It's not critical, just thought I'd bring this to your attention.
Thanks everyone for putting this fix together and reviewing!

@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

@bergundy I didn't look into the performance case of that :( Should I open another PR with the Promise approach?

@bergundy
Copy link

It’s really not too critical imho, up to you.

@bergundy
Copy link

Here’s more information about the difference between tasks and microtasks FTR: https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

That's was a good read! thanks for sharing 😊

@lukekarrys Do you know how long that node 10 is going to be supported?

We could also leverage the promise conditionally depending on if the micro task is available if node 10 support is going to be around for longer than a couple of months.

@lukekarrys lukekarrys mentioned this pull request Oct 27, 2022
Merged
lukekarrys added a commit that referenced this pull request Oct 27, 2022
@lukekarrys
chore: use a local instead of remote file for test
767530e 
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 27, 2022
@lukekarrys
chore: use a local instead of remote file for test
341e750 
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
@bergundy
Copy link

I see a release PR was created by a bot: #326, can we get this approved please?

lukekarrys added a commit that referenced this pull request Oct 29, 2022
@lukekarrys
chore: use a local instead of remote file for test
60c3697 
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 29, 2022
@lukekarrys
chore: use a local instead of remote file for test
d0fedb1 
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 29, 2022
@lukekarrys
chore: use a local instead of remote file for test
2e45b11 
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
@lukekarrys
Copy link
Contributor

I see a release PR was created by a bot: #326, can we get this approved please?

This has been released as 6.1.12. Sorry for the delay, I wanted to do a few upstream tests in npm/cli first. Thanks to @webark for the fix and @bergundy for the review and help!

ksibisamir added a commit to SaTT-Wallet/Backend that referenced this pull request May 26, 2023
@ksibisamir
[Snyk] Upgrade tar from 6.1.11 to 6.1.14 (#212)
aa0fdad 
<h3>Snyk has created this PR to upgrade tar from 6.1.11 to 6.1.14.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

- The recommended version is **3 versions** ahead of your current
version.
- The recommended version was released **22 days ago**, on 2023-05-02.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>tar</b></summary>
    <ul>
      <li>
<b>6.1.14</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.14">2023-05-02</a></br><p>6.1.14</p>
      </li>
      <li>
<b>6.1.13</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.13">2022-12-07</a></br><h2><a
href="https://snyk.io/redirect/github/npm/node-tar/compare/v6.1.12...v6.1.13">6.1.13</a>
(2022-12-07)</h2>
<h3>Dependencies</h3>
<ul>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/cc4e0ddfe523a0bce383846a67442c637a65d486"><code>cc4e0dd</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/343"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/343/hovercard">#343</a> bump
minipass from 3.3.6 to 4.0.0</li>
</ul>
      </li>
      <li>
<b>6.1.12</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.12">2022-11-01</a></br><h2><a
href="https://snyk.io/redirect/github/npm/node-tar/compare/v6.1.11...v6.1.12">6.1.12</a>
(2022-10-31)</h2>
<h3>Bug Fixes</h3>
<ul>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/57493ee66ece50d62114e02914282fc37be3a91a"><code>57493ee</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/332"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/332/hovercard">#332</a>
ensuring close event is emited after stream has ended (<a
class="user-mention notranslate" data-hovercard-type="user"
data-hovercard-url="/users/webark/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/webark">@ webark</a>)</li>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/b003c64f624332e24e19b30dc011069bb6708680"><code>b003c64</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/314"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/314/hovercard">#314</a>
replace deprecated String.prototype.substr() (<a class="issue-link
js-issue-link" data-error-text="Failed to load title"
data-id="1192619366" data-permission-text="Title is private"
data-url="isaacs/node-tar#314"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/314/hovercard"
href="https://snyk.io/redirect/github/isaacs/node-tar/pull/314">#314</a>)
(<a class="user-mention notranslate" data-hovercard-type="user"
data-hovercard-url="/users/CommanderRoot/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/CommanderRoot">@
CommanderRoot</a>, <a class="user-mention notranslate"
data-hovercard-type="user"
data-hovercard-url="/users/lukekarrys/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/lukekarrys">@ lukekarrys</a>)</li>
</ul>
<h3>Documentation</h3>
<ul>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/f12992932f171ea248b27fad95e7d489a56d31ed"><code>f129929</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/313"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/313/hovercard">#313</a> remove
dead link to benchmarks (<a class="issue-link js-issue-link"
data-error-text="Failed to load title" data-id="1175996338"
data-permission-text="Title is private"
data-url="isaacs/node-tar#313"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/313/hovercard"
href="https://snyk.io/redirect/github/isaacs/node-tar/pull/313">#313</a>)
(<a class="user-mention notranslate" data-hovercard-type="user"
data-hovercard-url="/users/yetzt/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/yetzt">@ yetzt</a>)</li>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/c1faa9f44001dfb0bc7638b2850eb6058bd56a4a"><code>c1faa9f</code></a>
add examples/explanation of using tar.t (<a class="user-mention
notranslate" data-hovercard-type="user"
data-hovercard-url="/users/isaacs/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/isaacs">@ isaacs</a>)</li>
</ul>
      </li>
      <li>
<b>6.1.11</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.11">2021-08-26</a></br><p>6.1.11</p>
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases">tar
GitHub release notes</a>
  </details>
</details>


<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>tar</b></summary>
    <ul>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/4aaffc862f4e991f7965ecf6527072c4423ecb49">4aaffc8</a>
6.1.14</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/4cbdd674bfb2bda2769b94410650fd803e2f55ef">4cbdd67</a>
deps: [email protected]</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/75d3081ccf91853e13b7e0e28a077347b5a1fe3e">75d3081</a>
fix: update repository url in package.json</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/82bb3286a299903465a941bd70252843cf308a1e">82bb328</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/5f316363790f925d01a5809718b2958d0f3c0661">5f31636</a>
chore: bump @ npmcli/template-oss from 4.10.0 to 4.11.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/a044a87c6c7fb3ace4ea9bf903c63f0f15965398">a044a87</a>
chore: release 6.1.13 (#344)</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/cc4e0ddfe523a0bce383846a67442c637a65d486">cc4e0dd</a>
deps: bump minipass from 3.3.6 to 4.0.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/5dcfcb37fd5f7189be7ce63ef85ae3fbbc47da89">5dcfcb3</a>
chore: bump events-to-array from 1.1.2 to 2.0.3</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/329caed7d218f1784592f98380ff5a76968141ec">329caed</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/72f6e3915a80ee0b4c6e759412b1c460f156f62c">72f6e39</a>
chore: bump @ npmcli/template-oss from 4.8.0 to 4.10.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/001eafbfe77b10aa41c06081d7d3c9a3a7913240">001eafb</a>
chore: release 6.1.12</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/ac1026a69f9e0f5043a3f52c6f49c42b43b2066a">ac1026a</a>
chore: dry up template-oss config</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/2e45b112bdb6e88d32fa09b3eab2482637493b6c">2e45b11</a>
chore: use a local instead of remote file for test</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/79378ef9d044d0e992582f5a4768d90e4e2c1e3b">79378ef</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/eaea26d7d8dbd5b2c8236b64df0f56ae5704cf2b">eaea26d</a>
chore: bump @ npmcli/template-oss from 4.7.1 to 4.8.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/57493ee66ece50d62114e02914282fc37be3a91a">57493ee</a>
fix: ensuring close event is emited after stream has ended</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/1e3fadfedf9ea35da5dc7d70926a6864de6381cc">1e3fadf</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/24045dcefb1febc0d201566598ec9a378abe372d">24045dc</a>
chore: bump @ npmcli/template-oss from 4.6.2 to 4.7.1</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/b003c64f624332e24e19b30dc011069bb6708680">b003c64</a>
fix: replace deprecated String.prototype.substr() (#314)</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/d9edb344a1c799abfb5bc82e0f134865911160ab">d9edb34</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/c78c108abd93c4cd88373f7f6733f7c04f7f8a7f">c78c108</a>
chore: bump @ npmcli/template-oss from 4.6.1 to 4.6.2</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/2a49e7aaedc4138c6b94104bdaf8de56d86d7876">2a49e7a</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/39c60adaed54340c746c7ca68ff9e209ebe62110">39c60ad</a>
chore: bump @ npmcli/template-oss from 4.5.1 to 4.6.1</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/08cc1562bd1a80394f41eaf5c1c11d92176f8446">08cc156</a>
chore: bump @ npmcli/eslint-config from 3.1.0 to 4.0.0</li>
    </ul>

<a
href="https://snyk.io/redirect/github/isaacs/node-tar/compare/e573aeea19d4d650908b7f6bf0a1ad8dce9f1736...4aaffc862f4e991f7965ecf6527072c4423ecb49">Compare</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiIyMjI0ZTc5Mi00MGE2LTQxYWMtOWYxNS02NTFhZGIyOWVhNjkiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjIyMjRlNzkyLTQwYTYtNDFhYy05ZjE1LTY1MWFkYjI5ZWE2OSJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00/settings/integration?pkg&#x3D;tar&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"2224e792-40a6-41ac-9f15-651adb29ea69","prPublicId":"2224e792-40a6-41ac-9f15-651adb29ea69","dependencies":[{"name":"tar","from":"6.1.11","to":"6.1.14"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"b89486be-ad07-4d6c-a51a-2fa8a25baa00","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":3,"publishedDate":"2023-05-02T22:46:07.312Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]})
--->
@hedera-eng-infrastructure hedera-eng-infrastructure mentioned this pull request Jan 25, 2024
Merged
@seansund seansund mentioned this pull request May 24, 2024
Open
@cocobabayaga cocobabayaga mentioned this pull request May 24, 2024
Open
@leshk0de leshk0de mentioned this pull request May 24, 2024
Open
@paaschdigital paaschdigital mentioned this pull request Jun 8, 2024
Open
@Jircs1 Jircs1 mentioned this pull request Jun 10, 2024
Open
@NOUIY NOUIY mentioned this pull request Jun 19, 2024
Open
@seansund seansund mentioned this pull request Sep 8, 2024
Open
@roma8389 roma8389 mentioned this pull request Sep 8, 2024
Open
@seansund seansund mentioned this pull request Sep 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukekarrys lukekarrys lukekarrys left review comments

@fritzy fritzy fritzy left review comments

@bergundy bergundy bergundy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Pipeline stream unexpectly being closed when using extract
4 participants
@webark @lukekarrys @bergundy @fritzy