Skip to content

fetch, tls, and http fixes #24740

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.

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

Merged
merged 6 commits into from
Aug 8, 2025
Merged

fetch, tls, and http fixes #24740

merged 6 commits into from
Aug 8, 2025

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Aug 8, 2025

Follow-up from #24698. Progress towards #24732.

cc @ianprime0509

it's still not quite all the way there yet:

andy@bark ~/d/z/build-release (http-plus-fixes)> stage4/bin/zig fetch "git+https://gitlab.com/fjc/zigby.git#57d01bac78e1a152172d1daee4868593eb1c5156"
error: unable to unpack git files: InvalidHeader

error return trace:

/home/andy/dev/zig/src/Package/Fetch/git.zig:1133:34: 0x2ce8791 in read (main.zig)
            error.EndOfStream => return error.InvalidHeader,
                                 ^
/home/andy/dev/zig/src/Package/Fetch/git.zig:1369:25: 0x2cea36d in indexPackFirstPass (main.zig)
    const pack_header = try PackHeader.read(&pack_hashed.reader);
                        ^
/home/andy/dev/zig/src/Package/Fetch/git.zig:1271:27: 0x2cf04cb in indexPack (main.zig)
    const pack_checksum = try indexPackFirstPass(allocator, format, pack, &index_entries, &pending_deltas);
                          ^
/home/andy/dev/zig/src/Package/Fetch.zig:1364:13: 0x2cfcfd7 in unpackGitPack (main.zig)
            try git.indexPack(gpa, object_format, &pack_file_reader, &index_file_writer);
            ^
/home/andy/dev/zig/src/Package/Fetch.zig:1222:89: 0x268ba52 in unpackResource (main.zig)
        .git_pack => return unpackGitPack(f, tmp_directory.handle, &resource.git) catch unreachable,
                                                                                        ^

It's a bit counter-intuitive, but there are two streams here: the
implementation here, and the connected output stream.

When we say "unflushed" we mean don't flush the connected output stream
because that's managed externally. But an "end" operation should always
flush the implementation stream.
simplifies the logic & makes it respect limit
Comment on lines +298 to +299
.zstd => std.compress.zstd.default_window_len,
.gzip, .deflate => std.compress.flate.max_window_len,
Copy link
Collaborator

@squeek502 squeek502 Aug 8, 2025

Choose a reason for hiding this comment

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

General comment mostly borne out of my own confusion: this concept of "buffer capacity" for std.compress seems to be defined in different ways many different places, and some of them appear to disagree with eachother.

For zstd, these doc comments would lead me to believe that default_window_len is not enough:

/// The output buffer is asserted to have capacity for `window_len` plus
/// `zstd.block_size_max`.

/// When connecting `reader` to a `Writer`, `buffer` should be empty, and
/// `Writer.buffer` capacity has requirements based on `Options.window_len`.
///
/// Otherwise, `buffer` has those requirements.

For deflate, these constants are just generally confusing to me:

/// When decompressing, the output buffer is used as the history window, so
/// less than this may result in failure to decompress streams that were
/// compressed with a larger window.
pub const max_window_len = history_len * 2;
pub const history_len = 32768;

Seems like some more definitive recommended_buffer_size or min_buffer_size constant should be defined per-compression-algorithm and used consistently.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I've been figuring this stuff out as I go along and only just starting to get a handle of it. Once I'm confident in the flow chart I'll do another pass over the doc comments, constants, and make sure everything is clear and agrees with each other.

even in this PR you can see I changed the rules for Reader.stream

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 tried to do all this stuff in a branch and only inflict it on master once I had a fully done, coherent change, but it's just too massive. everybody has to come along for the ride and suffer with me, sorry

@andrewrk andrewrk enabled auto-merge August 8, 2025 06:10
@andrewrk andrewrk disabled auto-merge August 8, 2025 19:33
@andrewrk andrewrk merged commit 1ba6838 into master Aug 8, 2025
9 of 12 checks passed
@andrewrk andrewrk deleted the http-plus-fixes branch August 8, 2025 19:33
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.

2 participants