You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Ryan Baxter <rb...@gmail.com> on 2011/12/04 22:04:16 UTC
Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/
-----------------------------------------------------------
Review request for shindig, Jesse Ciancetta and Brian Lillie.
Summary
-------
After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
This addresses bug SHINDIG-1667.
https://issues.apache.org/jira/browse/SHINDIG-1667
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
Diff: https://reviews.apache.org/r/3006/diff
Testing
-------
Ran unit tests and rendered gadgets in common container
Thanks,
Ryan
Re: Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/#review3660
-----------------------------------------------------------
Committed revision 1210933.
- Ryan
On 2011-12-04 21:04:16, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3006/
> -----------------------------------------------------------
>
> (Updated 2011-12-04 21:04:16)
>
>
> Review request for shindig, Jesse Ciancetta and Brian Lillie.
>
>
> Summary
> -------
>
> After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
>
>
> This addresses bug SHINDIG-1667.
> https://issues.apache.org/jira/browse/SHINDIG-1667
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
>
> Diff: https://reviews.apache.org/r/3006/diff
>
>
> Testing
> -------
>
> Ran unit tests and rendered gadgets in common container
>
>
> Thanks,
>
> Ryan
>
>
Re: Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/#review3619
-----------------------------------------------------------
Ship it!
LGTM
- Jesse
On 2011-12-04 21:04:16, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3006/
> -----------------------------------------------------------
>
> (Updated 2011-12-04 21:04:16)
>
>
> Review request for shindig, Jesse Ciancetta and Brian Lillie.
>
>
> Summary
> -------
>
> After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
>
>
> This addresses bug SHINDIG-1667.
> https://issues.apache.org/jira/browse/SHINDIG-1667
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
>
> Diff: https://reviews.apache.org/r/3006/diff
>
>
> Testing
> -------
>
> Ran unit tests and rendered gadgets in common container
>
>
> Thanks,
>
> Ryan
>
>
Re: Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/#review3629
-----------------------------------------------------------
Ship it!
Never seen this one happening but good catch
- Henry
On 2011-12-04 21:04:16, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3006/
> -----------------------------------------------------------
>
> (Updated 2011-12-04 21:04:16)
>
>
> Review request for shindig, Jesse Ciancetta and Brian Lillie.
>
>
> Summary
> -------
>
> After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
>
>
> This addresses bug SHINDIG-1667.
> https://issues.apache.org/jira/browse/SHINDIG-1667
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
>
> Diff: https://reviews.apache.org/r/3006/diff
>
>
> Testing
> -------
>
> Ran unit tests and rendered gadgets in common container
>
>
> Thanks,
>
> Ryan
>
>
Re: Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
Posted by Jesse Ciancetta <jc...@mitre.org>.
> On 2011-12-05 19:15:25, Brian Lillie wrote:
> > In doFetchConcatResources, if one of the 2nd through nth requests encounters an error such that false would be returned, we still have the content from the 1st up to the failing request in the VerbatimConcatOutputStream, and when we do the close on that stream, it will be written out as well as the the 400 error code. Seems like the cases where that might occur are probably remote, and having extra data in the response shouldn't matter. Not sure that it needs to be fixed .. just pointing it out.
>
> Ryan Baxter wrote:
> Yeah I thought of that too Brian. I think setting the error code when we return should let be an indication to ignore the content in the response. Jesse would you agree?
Yup - I saw that too and thought it was odd but that's the way it worked before your changes so I'm fine with leaving it as is. At least with your changes in place we should get back proper 400 codes consistently even if the previously buffered content is large enough to fill the response buffer.
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/#review3628
-----------------------------------------------------------
On 2011-12-04 21:04:16, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3006/
> -----------------------------------------------------------
>
> (Updated 2011-12-04 21:04:16)
>
>
> Review request for shindig, Jesse Ciancetta and Brian Lillie.
>
>
> Summary
> -------
>
> After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
>
>
> This addresses bug SHINDIG-1667.
> https://issues.apache.org/jira/browse/SHINDIG-1667
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
>
> Diff: https://reviews.apache.org/r/3006/diff
>
>
> Testing
> -------
>
> Ran unit tests and rendered gadgets in common container
>
>
> Thanks,
>
> Ryan
>
>
Re: Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
Posted by Ryan Baxter <rb...@gmail.com>.
> On 2011-12-05 19:15:25, Brian Lillie wrote:
> > In doFetchConcatResources, if one of the 2nd through nth requests encounters an error such that false would be returned, we still have the content from the 1st up to the failing request in the VerbatimConcatOutputStream, and when we do the close on that stream, it will be written out as well as the the 400 error code. Seems like the cases where that might occur are probably remote, and having extra data in the response shouldn't matter. Not sure that it needs to be fixed .. just pointing it out.
Yeah I thought of that too Brian. I think setting the error code when we return should let be an indication to ignore the content in the response. Jesse would you agree?
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/#review3628
-----------------------------------------------------------
On 2011-12-04 21:04:16, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3006/
> -----------------------------------------------------------
>
> (Updated 2011-12-04 21:04:16)
>
>
> Review request for shindig, Jesse Ciancetta and Brian Lillie.
>
>
> Summary
> -------
>
> After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
>
>
> This addresses bug SHINDIG-1667.
> https://issues.apache.org/jira/browse/SHINDIG-1667
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
>
> Diff: https://reviews.apache.org/r/3006/diff
>
>
> Testing
> -------
>
> Ran unit tests and rendered gadgets in common container
>
>
> Thanks,
>
> Ryan
>
>
Re: Review Request: ConcatProxyServlet sets the HTTP response status after
writing the response
Posted by Brian Lillie <br...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3006/#review3628
-----------------------------------------------------------
In doFetchConcatResources, if one of the 2nd through nth requests encounters an error such that false would be returned, we still have the content from the 1st up to the failing request in the VerbatimConcatOutputStream, and when we do the close on that stream, it will be written out as well as the the 400 error code. Seems like the cases where that might occur are probably remote, and having extra data in the response shouldn't matter. Not sure that it needs to be fixed .. just pointing it out.
- Brian
On 2011-12-04 21:04:16, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3006/
> -----------------------------------------------------------
>
> (Updated 2011-12-04 21:04:16)
>
>
> Review request for shindig, Jesse Ciancetta and Brian Lillie.
>
>
> Summary
> -------
>
> After we return from doFetchConcatResources(..) we set the status in doGet. So lets say we return false from doFetchConcatResources and the content written to the response was bigger than the buffer. This means we will flush the buffer and set the status to the OK status then write the rest of the content and set the status to bad request, but it will be to late we already set the status to OK
>
>
> This addresses bug SHINDIG-1667.
> https://issues.apache.org/jira/browse/SHINDIG-1667
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java 1210178
>
> Diff: https://reviews.apache.org/r/3006/diff
>
>
> Testing
> -------
>
> Ran unit tests and rendered gadgets in common container
>
>
> Thanks,
>
> Ryan
>
>