-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
std: rework HTTP and TLS for new I/O API #24698
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
Conversation
628de83
to
309a945
Compare
Side note: I'll work on upstreaming all the resinator changes so far to double check its still working as intended before 0.15.0. |
Thanks. If you have the time and motivation, it would be nice to do a more thorough upgrade, too. For instance I noticed a lot of those seeks could become no-ops once they are using File.Reader API. |
Just a small observation, if I'm reading tls.Client and http.Reader code correctly. tls.Client stream method requires that the reader, which calls it, has enough unused capacity to fit cleartext record. If that unused capacity is less than the decoded record we will get error.OutputBufferUndersize. For 'safe' operation with tls.Client we must ensure to have tls.min_buffer_len of unused capacity on each call to stream method it is not enough to ensure that initial reader buffer is at least tls.min_buffer_len. For example, in https.Client if we get partial headers in the first tls record, http parser will call fillMore while leaving partial headers in the buffer, if the next tls record is big enough and reader buffer is initialized with tls.min_buffer_len, decrypted record can't fit into unused space. This makes some extra requirements for sizing tls.Client read buffer. |
Note that because the ziglang.org master builds were purged recently and |
what do you mean by this? |
not quite because it will rebase if necessary |
Uhhh, I assumed this was intentional? The 0.15.0 dev build which |
Ah, I see, I didn't know you'd automated that -- thanks for pointing it out! |
Please ignore if this is only my misunderstanding... I'm thinking about something like this: |
@ianic yes you are right, thank you for laying it out so plainly. In this case the buffer size requirement is cumulative. |
b7a94cf
to
aac26f3
Compare
Consistently hitting Example urls that fail:
Can also reproduce it with const std = @import("std");
pub fn main() !void {
var debug_allocator: std.heap.DebugAllocator(.{}) = .{};
defer std.debug.assert(debug_allocator.deinit() == .ok);
const gpa = debug_allocator.allocator();
var client: std.http.Client = .{ .allocator = gpa };
defer client.deinit();
var body: std.ArrayListUnmanaged(u8) = .empty;
defer body.deinit(gpa);
const res = try client.fetch(.{
.location = .{ .url = "https://www.ryanliptak.com/misc/notin.html" },
.method = .GET,
.response_storage = .{ .allocator = gpa, .list = &body },
});
std.debug.print("{}\n", .{res.status});
}
EDIT: Same
|
it's not quite finished because I need to make it not copy the Resource
Now it avoids mutating `r` unnecessarily, allowing the `ending` Reader to work.
I never liked how this data structure took its API as a parameter. This use case is now served by std.Io buffering.
Just enough to get things working correctly again
and fix bad unit test API usage that it finds
thank you everybody
let's see if anybody notices it missing
respect the case when there is existing buffer
* TLS: add missing assert for output buffer length requirement * TLS: add missing flushes * TLS: add flush implementation * TLS: finish drain implementation * HTTP: correct buffer sizes for TLS * HTTP: expose a getReadError method on Connection * HTTP: add missing flush on sendBodyComplete * Fetch: remove unwanted deinit * Fetch: improve error reporting
We're already in a regressed state relative to a couple weeks back, so I'm going to let this through since it's progress towards towards the solution, and #24732 blocks the release. |
var adapter = resource.reader().adaptToNewApi(&adapter_buffer); | ||
var decompress: std.compress.zstd.Decompress = .init(&adapter.new_interface, window_buffer, .{ | ||
var decompress: std.compress.zstd.Decompress = .init(resource.reader(), window_buffer, .{ | ||
.verify_checksum = false, | ||
}); | ||
return try unpackTarball(f, tmp_directory.handle, &decompress.reader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the changes in this PR, but this won't work for all tar.zst
files, see #24735
Followup from #24329.
Bonus breakage:
std.Io.LimitedReader
std.Io.BufferedReader
std.fifo
Performance Data Points
Compiler Binary Size (ReleaseSmall)
13.5 -> 13.4 MiB (-1%)
Merge checklist