Opened 16 years ago

Closed 11 years ago

#1937 enhancement closed wontfix (wontfix)

in twisted.web2, change "stream" to use newfangled not yet defined stream api

Reported by: Glyph Owned by: dialtone
Priority: normal Milestone:
Component: web2 Keywords:
Cc: zooko@… Branch: branches/newstream-1937-3
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

I could swear I made a ticket about this already, but I couldn't find it. If someone finds it, please close this as a duplicate.

twisted.web2.stream.IStream is an unnecessary duplication and inversion of the IConsumer API. The readStream convenience API and _StreamReader helper demonstrate the basic problem, by inverting the slow, "convenient" deferred-per-block-of-data API and turning it into IProtocol, simultaneously destroying the ability to handle errors cleanly by putting data-handling into a callback but error-handling into a Deferred. In order to implement the obvious state machine you have to map it onto something even more like IProtocol... etc.

All this code will be removed before a final release of web2; I believe we will simply use the existing interfaces rather than try to invent a new abstraction. Since much of the code using streams is already turning them inside out to get something that looks more like IProtocol this should not be too difficult, and we can have a transitional IStream to split the work into smaller chunks, now that web2.dav depends upon it so heavily.

Change History (20)

comment:1 Changed 16 years ago by jknight

I disagree, I tried doing it with normal producers first. You might even be able to find that code in old svn history for stream.py. It made the interface ugly, and made the code even more complicated and hard to follow.

But nothing I say will prevent you from repeatedly stating that you're going to try to do it some day, so, oh well.

comment:2 Changed 16 years ago by jknight

<glyph> Vito`: at least one major breakage is planned: http://twistedmatrix.com/trac/ticket/1937
<foom> *sigh*
<foom> "is planned" is such an overstatement. "is still hanging over web2's head like a guillotine", maybe.
<glyph> foom: Streams suck.  Producers might not be great either, but suckage is multiplicative, and if we have producers that suck and then streams that suck and then some random new API special-purpose for streaming over some other protocol (XMPP?) then we are going to have suck**3.
<glyph> foom: I agree that we should fix the issues with producers too, but the fix is not specific to web (and I don't believe streams are a fix)
<foom> glyph: I don't either
<foom> glyph: I intended streams to migrate back into twisted
<foom> glyph: (repsonding to "web-only", that is)
<dialtone> I fail to understand why streams are so bad...
<glyph> foom: Hmm.  I am really busy today, but we should probably slog this out on the ticket, so that we stop repeating ourselves.

comment:3 Changed 16 years ago by Glyph

Summary: remove twisted.web2.stream, replace with regular consumer/producer APIchange twisted.web2.stream to use regular consumer/producer/protocol API, rather than "read() -> Deferred"

So, I was looking at stream.py after the previous IRC exchange and I had an epiphany.

It's not streams that I see as the problem. Reviewing the code to refresh my memory, I finally understood what all this stuff is for. My basic problem remains the same, but I'm renaming the ticket to reflect what is probably a better general decision.

I notice that very few application-level features actually call read(), and about 90% of the code that does call read() just turns it into _gotData and _gotError anyway -- better known as "dataReceived" and "connectionLost" elsewhere in the codebase. The higher-level readStream API, I don't think is great, but is also the sort of wrapper functionality that will (A) continue to work if the "stream" object has some alien interface which requires an IProtocol provider, and (B) is not the kind of thing that I think creates a serious problem that should block anybody releasing or stabilizing anything.

With this refined understanding of what I think needs to happen, I've softened the title of the ticket and as I figure out a more exact solution I will probably change the description to suggest the new interface. I think the IStream interface will stick around, and flip around from "pulling" data to "pushing" data, with some convenience APIs for hooking up to IConsumer providers on the other end.

Hopefully I can describe this in terms that someone else will find favorable enough to implement...

comment:4 Changed 16 years ago by Glyph

Priority: highlow
Summary: change twisted.web2.stream to use regular consumer/producer/protocol API, rather than "read() -> Deferred"in twisted.web3, change "stream" to use regular consumer/producer/protocol API, rather than "read() -> Deferred"

Well, IStream is a terrible API, and I wish we could have dealt with this in a timely manner. However, Apple has decided to base Calendar Server on web2, and I think that means that it is time to do a release and start supporting web2's API.

I'm a bit disappointed in the extremely meager level of investment Apple has made in web2, and Twisted in general, but it seems like this could be good press; immediately breaking all the APIs they're using would not be.

Time for web3...

comment:5 in reply to:  4 Changed 16 years ago by Wilfredo Sánchez Vega

Replying to glyph:

Calendar Server is not dependant on Twisted releasing web2; we're embedding a specific revision from svn already. Further, you should feel free to fix this API as appropriate, and we'd be happy to adapt to it.

By the way, web2.dav took me a lot of time, and was done on Apple time which was donated to Twisted. I think "extremely meager level of investment" is a bit of a mischaracterization of Apple's involvement to date.

comment:6 in reply to:  4 Changed 16 years ago by jknight

Summary: in twisted.web3, change "stream" to use regular consumer/producer/protocol API, rather than "read() -> Deferred"in twisted.web2, change "stream" to use newfangled not yet defined stream api

I, for one am very happy with Apple and wsanchez's contributions to Twisted. And glyph is too, he just doesn't know how to say it properly in bug reports.

<glyph> wsanchez: I appreciate the level of investment that has been made. I am really happy that you've contributed web2.dav.

comment:7 Changed 16 years ago by Glyph

Priority: lownormal

comment:8 Changed 15 years ago by Glyph

Owner: changed from Glyph to jknight

comment:9 Changed 15 years ago by therve

Component: coreweb2

comment:10 Changed 15 years ago by jknight

I started working on this a while ago on a branch somewhere. But I'm going to sleep now instead of looking for it.

comment:11 Changed 15 years ago by jknight

Hey, it was named correctly, so pretty easy to find: web2-new-stream-1937

comment:12 Changed 15 years ago by jknight

I guess I should linkify it: source:branches/web2-new-stream-1937

comment:13 Changed 15 years ago by Glyph

Owner: changed from jknight to dialtone

This should probably be assigned to dialtone, since he has repeatedly voiced an interest in working on it.

comment:14 Changed 15 years ago by David Reid

Branch: web2-new-stream-1937-2

comment:15 Changed 14 years ago by Glyph

author: glyph
Branch: web2-new-stream-1937-2branches/newstream-1937-3

(In [22871]) Branching to 'newstream-1937-3'

comment:16 Changed 12 years ago by zooko

Hey there, so suppose I wanted to gain incremental access to a long stream of data that was being uploaded to my twisted web server in a long POST. How should I do that? Here is why: http://tahoe-lafs.org/trac/tahoe-lafs/ticket/320 (add streaming (on-line) upload to HTTP interface).

comment:17 Changed 12 years ago by Jean-Paul Calderone

You may be more interested in #288.

comment:18 Changed 12 years ago by zooko

You're right, I'm more interested in #288.

comment:19 Changed 11 years ago by zooko

Cc: zooko@… added

comment:20 Changed 11 years ago by Jean-Paul Calderone

Resolution: wontfix
Status: newclosed

Not going to change web2 to use something different from streams (see #4821).

Note: See TracTickets for help on using tickets.