Skip to content
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

std: Add std.http.headers module #2263

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Conversation

daurnimator
Copy link
Contributor

@daurnimator daurnimator commented Apr 12, 2019

This is a first step towards #910 and #2007
As I mentioned at #910 (comment)

I think the networking side of this issue needs to wait for #1778. However there is a small amount of work that can be done before then including e.g. adding a headers object.

The headers object here can be used by any/all of a http client, proxy or server.
A client will eventually have a request object with properties including this http header object, the response would be in the form of another http headers object + a body stream.
A server would carefully (e.g. blocking over-long lines + limiting number of entries) read headers from the wire and build up a headers object. A request handler would create a response headers object and write it back to the http stream.

Note this PR is on top of #2262, so merge that first.

@daurnimator
Copy link
Contributor Author

Test failed with OutOfMemory on windows in the new code.... is there an actual issue or is it just some CI side-effect?

@daurnimator daurnimator force-pushed the http.headers branch 2 times, most recently from ebf1954 to 1d2231e Compare April 22, 2019 15:06
@daurnimator
Copy link
Contributor Author

Test failed with OutOfMemory on windows in the new code.... is there an actual issue or is it just some CI side-effect?

It was an issue in this new code. Tracked down and fixed the missing deallocation thanks to #2338

Copy link
Contributor

@jayschwa jayschwa left a comment

Choose a reason for hiding this comment

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

HTTP header names are case-insensitive according to the specification. I only skimmed the PR, but it looks like this implementation is case-sensitive.

@daurnimator
Copy link
Contributor Author

HTTP header names are case-insensitive according to the specification

Yes. However you don't want to reflect this in your headers data structure

  1. Some (buggy) servers rely on certain cases for certain fields. This is common enough that http libraries need a way to specify the case on a per header basis
  2. e.g. To implement a WAF, you need the unnormalised headers

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

This is going in a different direction than I think makes sense for Zig.

I would expect the first step to be a non-allocating streaming API, with any higher level API built on top of that one. The streaming API should be feature complete before doing any higher level abstraction. For example, it should be able to handle http 1.1 (or whatever is the oldest version zig std lib chooses to support) as well as 2. It would be recommended to use the streaming API for most use cases, with zig potentially not even providing a "container" API which this pull request implements.

Zig's claim to be "optimal" is not an empty promise - we have to build our APIs on top of this premise.

You can see @tiehuis's work with the std lib json API for an example of this methodology.

@daurnimator
Copy link
Contributor Author

I would expect the first step to be a non-allocating streaming API

Such an API isn't really possible for HTTP/2: in HTTP/2 you must decode headers as they arrive due to HPACK compression state. And they need to be decoded into something like this PR's container object.

The streaming API should be feature complete before doing any higher level abstraction

A streaming API here really needs the coroutine rewrite to be complete first. This PR was trying to get some work done before copy elision/coroutines are ready.

For HTTP/1 where a streaming API is at least possible, I imagine that the main action would be calling .append from this PR.

Note that once this PR is merged, a HPACK module can be started on.

This is going in a different direction than I think makes sense for Zig.

I think that any usable HTTP client library will need a HTTP container structure just as this PR provides. I'm not sure what you would propose instead.

Based on lua-http's data structure
@daurnimator
Copy link
Contributor Author

daurnimator commented Jun 11, 2019

I can't figure out why the CI tests are failing.... with #2337 applied locally I get:

9/12 test "Headers.get"...allocation of 384 success!
allocation of 3 success!
allocation of 3 success!
allocation of 64 success!
allocation of 1024 success!
allocation of 10 success!
allocation of 3 success!
allocation of 64 success!
allocation of 3 success!
allocation of 48 success!
free of 48 bytes success!
allocation of 96 success!
free of 96 bytes success!
free of 64 bytes success!
free of 10 bytes success!
free of 64 bytes success!
free of 3 bytes success!
free of 1024 bytes success!
free of 3 bytes success!
free of 3 bytes success!
free of 3 bytes success!
free of 384 bytes success!
OK

Which perfectly cancels out...

@andrewrk
Copy link
Member

I have some opinions about the http APIs of the standard library and I'm going to be involved in designing and iterating on the APIs. This is a different direction than I want to explore first.

I would encourage you, if you have strong opinions on this (which it seems like you do), to have a separate repo with this implementation & all the http related APIs how you want them. It won't hurt to have 2 competing APIs and we can compare the merits of them, especially once #2377 is complete. Probably the final result would be somewhere in between.

@andrewrk andrewrk closed this Jun 27, 2019
@andrewrk andrewrk reopened this Jun 27, 2019
@andrewrk andrewrk merged commit 43d52fa into ziglang:master Jun 27, 2019
@andrewrk
Copy link
Member

<andrewrk> daurnimator, I can appreciate the situation you're in, you've already done a bunch of work, you've read the spec, you have some working code with tests
<andrewrk> I'm willing to let you lead the charge on http stuff in the std lib, but you have to be willing to accept it is something that I will personally be involved in (probably once coroutine rewrite is done) and that I may decide to take the API in a different, possibly breaking, direction
<andrewrk> if you are ok with this, I'll walk back my decision on your PR and merge it (once the tests are passing)
<daurnimator> andrewrk: I guess I'm okay with it.... it's better than the alternative of nothing

@daurnimator daurnimator deleted the http.headers branch June 27, 2019 18:21
@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Aug 2, 2019
@marler8997
Copy link
Contributor

This API isn't something I would use in my ziget project (https://github.com/marler8997/ziget) which is meant to provide functionality to download files via HTTP. This API seems meant to aid in higher level HTTP applications that need to make alot more use of HTTP, but doesn't expose the lower-level functionality in such a way that simpler consumers can make use of without bringing in unnecessary overhead.

I don't need an allocator to parse the headers, and I don't need an allocator to store the headers. I just need a way to parse headers and find some of its values. All of ziget's HTTP specific parsing code along with tests is contained in this one module (https://github.com/marler8997/ziget/blob/master/ziget/http/parse.zig). I haven't looked at this API too closely, but I would suggest starting by exposing an API that can parse HTTP similar to what my module does there, and then build structures on top of it like what you've done here.

@andrewrk
Copy link
Member

I completely agree @marler8997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants