std: Fix using `fs.Dir.Iterator` twice #10404

ominitay · 2021-12-24T01:24:06Z

This fixes the use of multiple Iterators in a row on a directory. Previously, on many platforms, using an Iterator on an already-iterated directory would give no entries.

Fixing this involved seeking to the beginning of the directory on the first call of next().

cc: @nektro @squeek502
fixes #10317

kubkon · 2022-01-22T23:24:34Z

I take it that #10317 is still not fixed hence why the CI fails with this test added? If that's the case, I see two solutions for this PR to be in mergable state: 1) if you're up for it, you could propose a fix to #10317, or 2) disable this test for the time being an leave a TODO comment referencing the issue. Lemme know what you think!

nektro · 2022-01-22T23:30:19Z

i think // TODO https://github.com/ziglang/zig/issues/10317 with a skip is a good route, there's precedence

ominitay · 2022-01-22T23:57:06Z

Yep, sorry, my PRs have fallen behind. I'll get this sorted in a moment to be merged.

ominitay · 2022-01-23T10:28:21Z

I'd like to see the Windows CI pass before this is merged, to be sure :)

ominitay · 2022-01-23T13:47:06Z

Ok, ready to merge! @kubkon

lib/std/fs.zig

kubkon · 2022-01-27T10:09:21Z

lib/std/fs.zig

@@ -369,6 +370,10 @@ pub const Dir = struct {
            fn nextSolaris(self: *Self) !?Entry {
                start_over: while (true) {
                    if (self.index >= self.end_index) {
+                        if (self.first) {
+                            std.os.lseek_SET(self.dir.fs, 0) catch unreachable;


Are we sure this cannot fail?

It definitely can't on Linux, I assumed POSIX compliant systems would be the same, though I just pushed this to see how the tests run.

I think an explanatory comment on each catch unreachable would be nice, but the possible errors for each platform are (from lseek(2)):

EBADF when Dir.fd is invalid or wasn't opened with iteration. This catch unreachable would show up later anyway during the next call (see here and other similar lines), so it being triggered during the lseek_SET just moves where it happens.

EINVAL when "whence is not valid or the resulting file offset would be negative, or beyond the end of a seekable device.", but we're always using the known-to-be-valid SEEK_SET and the offset is always 0

EOVERFLOW when "The resulting file offset cannot be represented in an off_t", but we know 0 will be the resulting file offset

ENXIO when lseek is using SEEK_DATA or SEEK_HOLE (actually, this isn't even a possible error within lseek_SET since we know we're using SEEK_SET)

ESPIPE when "fd is associated with a pipe, socket, or FIFO.", but we know fd is a directory

WASI has an additional ENOTCAPABLE that I'm unsure about when/if it can be triggered.

Hmm, I see. Curious to see if it applies to BSD/macOS too. I remember in the past I had some mixed experience with Dir iterators on macOS - the way the dir pointer works was completely different to how it works on Linux. But let's see if the tests pass. If so, then we're golden!

Yeah, worth looking further into this.

I think an explanatory comment on each catch unreachable would be nice, but the possible errors for each platform are (from lseek(2)):

EBADF when Dir.fd is invalid or wasn't opened with iteration. This catch unreachable would show up later anyway during the next call (see here and other similar lines), so it being triggered during the lseek_SET just moves where it happens.

EINVAL when "whence is not valid or the resulting file offset would be negative, or beyond the end of a seekable device.", but we're always using the known-to-be-valid SEEK_SET and the offset is always 0

EOVERFLOW when "The resulting file offset cannot be represented in an off_t", but we know 0 will be the resulting file offset

ENXIO when lseek is using SEEK_DATA or SEEK_HOLE (actually, this isn't even a possible error within lseek_SET since we know we're using SEEK_SET)

ESPIPE when "fd is associated with a pipe, socket, or FIFO.", but we know fd is a directory

WASI has an additional ENOTCAPABLE that I'm unsure about when/if it can be triggered.

ENOTCAPABLE would be triggered when we want iterate a dir tree we weren't given capabilities to do so.

lib/std/fs.zig

ominitay · 2022-01-27T13:44:13Z

Made changes, noticed I skipped over WASI and Darwin, so they should be sorted now :)

ominitay · 2022-01-27T16:38:00Z

Am confused what went on with the Linux CI there. Seems to have failed on code which I didn't touch :/
My code seemed to pass fine though.

ominitay · 2022-01-27T19:38:09Z

Will just push this to rerun CI

nektro · 2022-01-27T20:18:26Z

maybe rebase

ominitay · 2022-01-27T20:55:18Z

maybe rebase

Already did that. This is so odd. Maybe I'll try reverting the changes I did to the WASI impl, see if that does anything...

ominitay · 2022-01-27T22:09:30Z

Seems to have done it. Somehow using lseek_SET in WASI broke unrelated things ...

squeek502 · 2022-01-28T14:46:16Z

It looks like lseek_SET is unecessary for WASI, and the rewinding is already taken care of by using DIRCOOKIE_START when creating the Iterator here:

zig/lib/std/fs.zig

Line 789 in 0e6d218

.cookie = os.wasi.DIRCOOKIE_START,

which then gets used when calling fd_readdir (note: not getdents like other platforms)

fd_readdir and dircookie docs:

The current implementation with no changes already passes the Dir.Iterator twice test case.

ominitay · 2022-01-28T15:48:46Z

Fantastic, thank you for explaining that to me!

With that, this should be ready to merge.

lib/std/fs.zig

This fixes the use of multiple `Iterator`s in a row on a directory. Previously, on many platforms, using an `Iterator` on an already-iterated directory would give no entries. Fixing this involved seeking to the beginning of the directory on the first call of `next()`.

kubkon

Looks great @ominitay ! Thank you for seeing it through!

std: Fix using `fs.Dir.Iterator` twice

ominitay force-pushed the iterator branch from 945072d to 0d419e9 Compare January 23, 2022 10:27

ominitay changed the title ~~Add test for using fs.Dir.Iterator twice~~ std: Fix using fs.Dir.Iterator twice Jan 27, 2022

kubkon reviewed Jan 27, 2022

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

kubkon reviewed Jan 27, 2022

View reviewed changes

ominitay force-pushed the iterator branch from c4627f5 to 45e7724 Compare January 27, 2022 10:24

squeek502 reviewed Jan 27, 2022

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

lib/std/fs.zig Outdated Show resolved Hide resolved

ominitay force-pushed the iterator branch from 45e7724 to 2538ca0 Compare January 27, 2022 13:43

ominitay force-pushed the iterator branch from 2538ca0 to 57b1c49 Compare January 27, 2022 19:39

Add test for using fs.Dir.Iterator twice

c3ef85a

ominitay force-pushed the iterator branch 2 times, most recently from 7674ef6 to e3260c2 Compare January 27, 2022 21:40

squeek502 reviewed Jan 28, 2022

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

squeek502 reviewed Jan 28, 2022

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

ominitay force-pushed the iterator branch from e3260c2 to dc11fe4 Compare January 28, 2022 16:12

Vexu approved these changes Jan 29, 2022

View reviewed changes

kubkon approved these changes Jan 30, 2022

View reviewed changes

kubkon merged commit dd7309b into ziglang:master Jan 30, 2022

ominitay deleted the iterator branch January 30, 2022 14:13

andrewrk pushed a commit that referenced this pull request Feb 3, 2022

Merge pull request #10404 from ominitay/iterator

ac3ac25

std: Fix using `fs.Dir.Iterator` twice

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: Fix using `fs.Dir.Iterator` twice #10404

std: Fix using `fs.Dir.Iterator` twice #10404

ominitay commented Dec 24, 2021 •

edited

kubkon commented Jan 22, 2022

nektro commented Jan 22, 2022 •

edited

ominitay commented Jan 22, 2022

ominitay commented Jan 23, 2022

ominitay commented Jan 23, 2022

kubkon Jan 27, 2022

ominitay Jan 27, 2022

squeek502 Jan 27, 2022 •

edited

kubkon Jan 27, 2022

ominitay Jan 27, 2022

kubkon Jan 27, 2022

ominitay commented Jan 27, 2022

ominitay commented Jan 27, 2022

ominitay commented Jan 27, 2022

nektro commented Jan 27, 2022

ominitay commented Jan 27, 2022

ominitay commented Jan 27, 2022

squeek502 commented Jan 28, 2022 •

edited

ominitay commented Jan 28, 2022

kubkon left a comment

std: Fix using fs.Dir.Iterator twice #10404

std: Fix using fs.Dir.Iterator twice #10404

Conversation

ominitay commented Dec 24, 2021 • edited

kubkon commented Jan 22, 2022

nektro commented Jan 22, 2022 • edited

ominitay commented Jan 22, 2022

ominitay commented Jan 23, 2022

ominitay commented Jan 23, 2022

kubkon Jan 27, 2022

Choose a reason for hiding this comment

ominitay Jan 27, 2022

Choose a reason for hiding this comment

squeek502 Jan 27, 2022 • edited

Choose a reason for hiding this comment

kubkon Jan 27, 2022

Choose a reason for hiding this comment

ominitay Jan 27, 2022

Choose a reason for hiding this comment

kubkon Jan 27, 2022

Choose a reason for hiding this comment

ominitay commented Jan 27, 2022

ominitay commented Jan 27, 2022

ominitay commented Jan 27, 2022

nektro commented Jan 27, 2022

ominitay commented Jan 27, 2022

ominitay commented Jan 27, 2022

squeek502 commented Jan 28, 2022 • edited

ominitay commented Jan 28, 2022

kubkon left a comment

Choose a reason for hiding this comment

std: Fix using `fs.Dir.Iterator` twice #10404

std: Fix using `fs.Dir.Iterator` twice #10404

ominitay commented Dec 24, 2021 •

edited

nektro commented Jan 22, 2022 •

edited

squeek502 Jan 27, 2022 •

edited

squeek502 commented Jan 28, 2022 •

edited