dir-reader: account for entries being changed after _read #50
Conversation
If this.entries is changed after _read() has been called, we will be out of sync and try to access an invalid index of this.entries. When the entry cannot be found, we emit end and close, which can drop files from reading.
|
This is great work, and something like it will be incorporated into fstream with credit to you, but I can't land it until there are meaningful tests to prevent regressions, and without doing some more digging to ensure that the underlying race condition is well and truly addressed. As @addaleax pointed out on #node-dev, this fixes the test cases in https://github.com/othiym23/eliminate-5082, which is a very good sign. However, as this code base is very complicated, I want to be careful. That said, I believe it's on the CLI team to come up with the tests for this, because the current test suite pretty much needs to be rewritten out of its current whimsical style and into something that gives us a better sense of how much of the code is covered by the tests, and this is part of that. I'm going to start working on a new test suite for it immediately, because I'd like to get this fix landed and released in next week's npm. The current state of things is not great both for npm and for Node 6 users. Thanks so much for your time and the patch, and to @addaleax for doing the bisection to find where the change to Node landed! |
|
Cool, happy to help. I'll keep trying to come up with a standalone, reproducible test case for it for the time being |
|
Yeah, either of you, let me know if I can do something to help – I have access to a machine where I can reproduce the bug (almost?) 100 % of the time with Node v6 (at least for the eliminate-5082 repo). |
|
I tested this patch and it does fix the npm/npm#5082 issue for me. But I did find a side effect. The .npmignore filter no longer prevents my .editorconfig and .npmignore files from being included in the package. It's a 100% repro for my project in my Arch Linux environment using the same repro steps I listed in npm/npm#12542. EnvironmentReproduction steps
ConsoleNotes
That said, having extra files in the package is far better than randomly excluding files silently. I still vote that you land this PR asap. |
If this.entries is changed after _read() has been called, we will be out of sync and try to access an invalid index of this.entries. When the entry cannot be found, we emit end and close, which can drop files from reading. (At least partially) fixes npm/npm#5082. Credit: @evanlucas Reviewed-By: @othiym23 PR-URL: #50
|
Reworded commit message and landed as a55ae72 / published as |
|
It turns out this is causing some other problems within npm and / or |
|
So, i've spent some time looking more into this. I'm not sure how we can actually make it 100% unless we double pass all of the entries. fstream-ignore modifies what entries are emitted up, but there is no guarantee that we have not already emitted a file when we read an ignore file. So, in order to make it 100% we would either have to filter all of the entries that need filtering prior to emitting any, or have a way to back out an entry in node-tar (which could already be an option, I just haven't seen it yet) |
|
Thanks for looking into this @evanlucas. I suspected that something like this may have been the case. I'll look into finding a way to make this work, but I would vastly prefer to have some kind of strategy that decides synchronously, and once, whether a given path should be included or not. Right now this is all feeling pretty rickety. |
Partially addresses a race condition that caused missing files during publish. Credit: @evanlucas PR-URL: npm/fstream#50
If this.entries is changed after _read() has been called, we will be
out of sync and try to access an invalid index of this.entries. When
the entry cannot be found, we emit end and close, which can drop files
from reading.
Apologies for no test...I am having trouble reproducing with actually using npm at this point. Thanks!
Should fix npm/npm#5082
Thanks!