Uh oh!
There was an error while loading. Please reload this page.
[v13.x backport] fs: fix WriteStream autoClose order - #32163
Conversation
6b59787 to
93889a5Compare93889a5 to
d696dbbComparenodejs-github-bot
commented
Mar 9, 2020
MylesBorins
commented
Mar 9, 2020
hey @ronag thanks for putting this together. I noticed that you modified the original meta data of the commit in this PR to include the backport PR URL and removed the original sign offs. For backport PRs it would be best if you could leave all the original commit meta data in place. Our tooling relies on that information for generating changelogs and it is a little more work for us to dig up the original meta data vs simply adding the Backport-PR-URL when ladning. FWIW don't worry about it for this PR (or the others you just opened) I'll fix that when landing, it is easy enough. Very much appreciate the initiative here though! |
d696dbb to
8519946CompareThere was a problem hiding this comment.
This needs to be updated since the timing changes, which is the purpose of the original fix.
nodejs-github-bot
commented
Mar 10, 2020
WriteStream autoClose was implemented by manually calling .destroy() instead of using autoDestroy and callback. This caused some invariants related to order of events to be broken. Fixes: nodejs#31776 PR-URL: nodejs#31790 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Backport-PR-URL: nodejs#32163
8519946 to
c515e98Comparenodejs-github-bot
commented
Mar 10, 2020
MylesBorins
commented
Mar 10, 2020
Looks like OSX failures are consistent (compare the CI & retry OSX runs in above comment) Here's a run against the current v13.x-staging branch as comparison |
@MylesBorins The failing tests seem totally unrelated though? Waiting for the referenced CI run to complete for comparison. |
ronag
commented
Mar 10, 2020
So test.sequential/test-timers-blocking-callback seems to be the difference, but again I can't see any relation to this PR. I'll run just one more CI to be sure. |
MylesBorins
commented
Mar 10, 2020
@ronag dgram uses streams doesn't it? maybe try rebasing against staging and running tests again? |
ronag
commented
Mar 10, 2020
This PR only affects |
MylesBorins
commented
Mar 10, 2020
Then it might be flakes that are now properly marked. Could you try rebasing and running CI one more time? |
ronag
commented
Mar 10, 2020
Yep, already done, see CI link above. |
WriteStream autoClose was implemented by manually calling .destroy() instead of using autoDestroy and callback. This caused some invariants related to order of events to be broken. Fixes: #31776 Backport-PR-URL: #32163 PR-URL: #31790 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins
commented
Mar 11, 2020
Fails are definitely unrelated the majority are already marked flaky but one isn't. Opened a PR to mark flaky Landed in 7eed9d6 |
PR-URL: #31790
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes