For consideration: base APIs supporting streaming fetch

This CL adds new stream-based APIs for getting content from and setting content to a MutableContent object, and extends this to the intended primary use, creation of HttpResponse objects.

It extends byte[]-mutation APIs by adding an internal wrapper around the byte array; however, no existing method's semantics are modified by this change, the wrapper providing a layer of indirection.

When using streaming-set/get APIs, the key limitation is put in place that a content stream may only be retrieved once. This (simplified) policy ensures that streaming content is accessed in a clear fashion. As well, when generating an HttpResponse from a content stream, byte[]-based content encoding detection is not supported. Adherence to these policies is intended, with this design approach (as opposed to building a completely parallel streaming pipeline), to be a function of wiring/configuration: proxies/rewriters that require streaming must *all* use streaming APIs.

The implementation allows a streaming-set operation to later be compatible with buffered-read operations. In other words, setContentStream(InputStream) is functionally equivalent to setContentBytes(byte[]), with the former buffering all content before supporting existing operations. A direct consequence is that HttpFetcher implementations may now be "streaming-type" without having to modify any existing rewriter logic.

The intent of this CL is to roll out support for streaming fetch operations in Shindig slowly:
1. As noted, allows HttpFetcher implementations to be streaming-type now.
2. Provides basis for streaming (pass-through) proxy support with little effort.
3. Provides basis for streaming rewrites (depending on type, will require additional work such as streaming parser).

Sending CL for design review before augmenting tests enforcing streaming policy.

[johnfargo, 2010-06-11 15:27:47]

Fixed patch.

[zhoresh, 2010-06-14 23:56:28]

And don't forget to write tests...

http://codereview.appspot.com/1626044/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/1626044/diff/5001/6002#newcode498
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:498:
if (body == null || !(body instanceof MutableContent.ByteArrayContentBytes)) {
I would postpond detection to the time you need it.
The encoding is really needed only in the case of batch request, so at that
point you should have all data.

http://codereview.appspot.com/1626044/diff/5001/6003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
(right):

http://codereview.appspot.com/1626044/diff/5001/6003#newcode296
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:296:
bufferedStream = IOUtils.toByteArray(getStream());
What if someone already ask to get the stream?

http://codereview.appspot.com/1626044/diff/5001/6003#newcode301
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:301:
return bufferedStream;
Set streamReturned to true

http://codereview.appspot.com/1626044/diff/5001/6003#newcode314
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:314:
return stream;
Set streamReturned to true

[johnfargo, 2010-06-15 00:54:30]

Thanks for the feedback, Ziv! Comments inline.

On Mon, Jun 14, 2010 at 4:56 PM, <zhoresh@gmail.com> wrote:

> And don't forget to write tests...
>
>
> http://codereview.appspot.com/1626044/diff/5001/6002
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
> (right):
>
> http://codereview.appspot.com/1626044/diff/5001/6002#newcode498
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:498:
> if (body == null || !(body instanceof
> MutableContent.ByteArrayContentBytes)) {
> I would postpond detection to the time you need it.
> The encoding is really needed only in the case of batch request, so at
> that point you should have all data.
>

It's actually "needed" in one critically important other way -- as a side
effect of setting the Content-Type for the HttpResponse object. About to
upload a patch that maintains these semantics while lazy-parsing the
encoding when needed.


>
> http://codereview.appspot.com/1626044/diff/5001/6003
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
> (right):
>
> http://codereview.appspot.com/1626044/diff/5001/6003#newcode296
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:296:
> bufferedStream = IOUtils.toByteArray(getStream());
> What if someone already ask to get the stream?
>

Barf, by design :)


>
> http://codereview.appspot.com/1626044/diff/5001/6003#newcode301
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:301:
> return bufferedStream;
> Set streamReturned to true
>

At this point, it's guaranteed to already be set to true.


>
> http://codereview.appspot.com/1626044/diff/5001/6003#newcode314
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:314:
> return stream;
> Set streamReturned to true
>

Done.


>
> http://codereview.appspot.com/1626044/show
>

[johnfargo, 2010-06-15 01:02:44]

Accommodate Ziv's comments. Lazy-detect encoding (still w/ header-set).

[zhoresh, 2010-06-15 04:58:30]

Some minor comments, mainly look good (tests...).
I still wonder if we have code path that do multiple getStream on the content. 
(I guess one way to test now would be to mimic the behavior in the byte array
content, and test the server)

http://codereview.appspot.com/1626044/diff/15001/16004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
(right):

http://codereview.appspot.com/1626044/diff/15001/16004#newcode293
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:293:
public byte[] getBytes() {
Add throw

http://codereview.appspot.com/1626044/diff/15001/16004#newcode304
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:304:
public InputStream getStream() {
add "throw StreamingException"

http://codereview.appspot.com/1626044/diff/15001/16004#newcode328
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:328:
return stream != other.stream;
is that always true?

[johnfargo, 2010-06-16 01:33:28]

On Mon, Jun 14, 2010 at 9:58 PM, <zhoresh@gmail.com> wrote:

> Some minor comments, mainly look good (tests...).
> I still wonder if we have code path that do multiple getStream on the
> content.
> (I guess one way to test now would be to mimic the behavior in the byte
> array content, and test the server)
>
>
> http://codereview.appspot.com/1626044/diff/15001/16004
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
> (right):
>
> http://codereview.appspot.com/1626044/diff/15001/16004#newcode293
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:293:
> public byte[] getBytes() {
> Add throw
>

It doesn't throw a checked exception though, just a RuntimeException. I
could add "throws StreamingException" all the way up the stack for
documentation purposes though... thoughts?


>
> http://codereview.appspot.com/1626044/diff/15001/16004#newcode304
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:304:
> public InputStream getStream() {
> add "throw StreamingException"
>

same


>
> http://codereview.appspot.com/1626044/diff/15001/16004#newcode328
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:328:
> return stream != other.stream;
> is that always true?


Not if the other StreamingContentBytes is constructed w/ a different
underlying stream.


>
>
> http://codereview.appspot.com/1626044/show
>