Opened 6 years ago

Last modified 6 years ago

#8143 enhancement new

HTTP/2 Part 1: New IRequest interface

Reported by: Cory Benfield Owned by: Cory Benfield
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch:


After some discussion with glyph, we decided to focus on building the new streaming APIs and shims first, and then plugging HTTP/2 into those, rather than continue working with the older twisted.web APIs.

For that reason, I'm going to open a number of tickets defining that API before I start dropping HTTP/2 into them. This is the first.

As discussed on the list, we need a new IRequest equivalent that is "streaming-first": that is, one that is not subject to the current assumption that the content will be provided ahead of time in its entirety. This is based almost entirely on the IResponse object that already exists for clients, with a number of unnecessary items removed and some of the wording changed.

Feedback is appreciated!

See also #7460.

Attachments (1)

istreamingrequest.patch (2.3 KB) - added by Cory Benfield 6 years ago.
IStreamingRequest, first draft patch.

Download all attachments as: .zip

Change History (4)

Changed 6 years ago by Cory Benfield

Attachment: istreamingrequest.patch added

IStreamingRequest, first draft patch.

comment:1 Changed 6 years ago by Adi Roiban

I saw that you have requested a review from Glyph, but I am sending my review anyway as this is waiting in the review queue for some time.

No pressure on Glyph, but I am trying to keep tickets in review for as short as possible :)

I am not sure how do you plan to use this interface. On the server side I would like to see a separation between the request and the response.

That is, the web server will receive input via an IRequest and send output via an IResponse.

The server will no longer send response related data using the IRequest.

In this case, code and phrase will no longer be part of the IRequest but will be part of an IResponse.

I am not sure how do you plan to use the version and why do we need it as a high level streaming interface :) For my conversion with you I remember that you were saying that the version is no longer relevant.

Do we need the length as part of the interface ? I assume that the information is already in the headers.

Length attribute can be left as an helper in the standard implementation.

When I first read the web client interface I found it hard to understand the "deliveryBody" name. After more reads of the docs and code it start to make sense, but for the new interface maybe we can find a better name.

For my something like 'setBodyConsumer' makes more sense than 'deliveryBody'

Until we have at least 3 implementation of this interface I would document the new interface as experimental, so that we can change it without all the backward compatibly effort.

For landing this interface I think that we need at least one implementation and narrative documentation advertising this new interface in the twisted.web docs.

The initial implementations can be something very simple, like streaming the whole content and storing in on a local file or parsing an url form encoded POST request

Once I have an idea and documentation about how do you want this interface to fit into twisted.web I can volunteer to implement a streaming multi-part form encoder as I already have an initial implementation based on my fork of twisted.web which does streaming.

Leaving this for review so that Glyph can take a look and send his feedback as I am not sure what you have discuses over IRC


comment:2 Changed 6 years ago by Glyph

Owner: changed from Glyph to Cory Benfield

Thanks for submitting this, Lukasa,

As such, it's not really a candidate for landing (there would need to be an implementation somewhere in here).

One minor quibble: on the wire, various parts of the HTTP protocol are bytes. They should be represented as bytes if they can contain undecodable nonsense. So it makes sense to me that "phrase" is bytes. However, "version" is interpreted sufficiently that portions have been decoded to integers; if you are going to have an error if it doesn't parse as an integer, then you should probably throw an error on non-ASCII as well, which means the user-facing API should actually be text.

I agree with Adi that we should be separating request and response. By not including anything like write or finish, IStreamingRequest implicitly separates request and response, and we'd just use iweb.IResponse for the response portion of this. But then it does include code, which doesn't make sense.

So I think this very much needs another go, at least to remove code and phrase.

Also: what makes length special? More specifically, what are we going to do with other headers that contain meaningfully structured values, like Accept and Content-Type and such? I think we need a more general plan for handling structured data in headers. (One possibility would actually be to have a separate interface that we could __conform__ to for each header that might be set; while that might be way too esoteric, we need some strategy that generalizes a little better.)

comment:3 Changed 6 years ago by Glyph

Keywords: review removed
Note: See TracTickets for help on using tickets.