[jetty-dev] setStatus() vs. sendError(), custom error feedback

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[jetty-dev] setStatus() vs. sendError(), custom error feedback

Christian Grün
Hi all,

in our project, we are using Jetty for various HTTP services. If
anything goes wrong, we are currently using the following code to set
the status code and send a plain error message:

 HttpServletResponse res = ... (Jetty  response)
 res.setStatus(code);
 res.getOutputStream().write(message);

This approach fails if we add a GzipFilter to the Jetty context. I
have read that sendError() should be used for all kinds of error
messages, but it returns a pre-formatted HTML page, while we'd prefer
a plain text message (as e.g. our REST services doesn't return HTML
by default). I assume that attaching a custom error message is the
correct way of dealing with the issue, but I wanted to ask if there is
a more convenient way you would recommend? If no, how should such
an error handler look like? Is it the best approach to just copy and
paste the original Jetty sources of sendError() and modify the
contents?

Thanks for the attention,

Christian
BaseX Team
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Greg Wilkins-5
Christian,

I see no reason why your approach would fail with a gzip filter?
Which filter do you use and how does it fail?  what status codes are
you trying to send?

regards

On 22 April 2012 23:34, Christian Grün <[hidden email]> wrote:

> Hi all,
>
> in our project, we are using Jetty for various HTTP services. If
> anything goes wrong, we are currently using the following code to set
> the status code and send a plain error message:
>
>  HttpServletResponse res = ... (Jetty  response)
>  res.setStatus(code);
>  res.getOutputStream().write(message);
>
> This approach fails if we add a GzipFilter to the Jetty context. I
> have read that sendError() should be used for all kinds of error
> messages, but it returns a pre-formatted HTML page, while we'd prefer
> a plain text message (as e.g. our REST services doesn't return HTML
> by default). I assume that attaching a custom error message is the
> correct way of dealing with the issue, but I wanted to ask if there is
> a more convenient way you would recommend? If no, how should such
> an error handler look like? Is it the best approach to just copy and
> paste the original Jetty sources of sendError() and modify the
> contents?
>
> Thanks for the attention,
>
> Christian
> BaseX Team
> _______________________________________________
> jetty-dev mailing list
> [hidden email]
> https://dev.eclipse.org/mailman/listinfo/jetty-dev
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Christian Grün
Greg,

thanks for your feedback. I'll first dump the (expected) error
feedback for a given REST request in our system:

$ curl -i "http://admin:admin@localhost:8984/rest?query=
HTTP/1.1 400 Bad Request
Content-Length: 53
Server: Jetty(6.1.26)

Stopped at line 1, column 1:
[XPST0003] Empty query.


The same request leads to an exception when called from a browser
(Chrome, Opera, etc):

java.lang.IllegalStateException
  at org.mortbay.servlet.GzipFilter$GzipStream.doNotGzip(GzipFilter.java:544)
  at org.mortbay.servlet.GzipFilter$GZIPResponseWrapper.noGzip(GzipFilter.java:372)
  at org.mortbay.servlet.GzipFilter$GZIPResponseWrapper.setStatus(GzipFilter.java:216)
  at org.basex.http.HTTPContext.status(HTTPContext.java:209)
  at org.basex.http.BaseXServlet.service(BaseXServlet.java:43)
  at javax.servlet.http.HttpServlet.service(HttpServlet.java:820)
  ...

This is how

a) we currently set up Jetty (the relevant line is currently commented
out in the source code):
  https://github.com/BaseXdb/basex-api/blob/master/src/main/java/org/basex/BaseXHTTP.java#L121

b) we send the status code:
  https://github.com/BaseXdb/basex-api/blob/master/src/main/java/org/basex/http/HTTPContext.java#L201

c) the .setStatus() call is triggered:
  https://github.com/BaseXdb/basex-api/blob/master/src/main/java/org/basex/http/BaseXServlet.java#L27

Do you think there's any clear mistake we've made? I'll be glad to
provide you with for more details,
Christian


> On 22 April 2012 23:34, Christian Grün <[hidden email]> wrote:
>> Hi all,
>>
>> in our project, we are using Jetty for various HTTP services. If
>> anything goes wrong, we are currently using the following code to set
>> the status code and send a plain error message:
>>
>>  HttpServletResponse res = ... (Jetty  response)
>>  res.setStatus(code);
>>  res.getOutputStream().write(message);
>>
>> This approach fails if we add a GzipFilter to the Jetty context. I
>> have read that sendError() should be used for all kinds of error
>> messages, but it returns a pre-formatted HTML page, while we'd prefer
>> a plain text message (as e.g. our REST services doesn't return HTML
>> by default). I assume that attaching a custom error message is the
>> correct way of dealing with the issue, but I wanted to ask if there is
>> a more convenient way you would recommend? If no, how should such
>> an error handler look like? Is it the best approach to just copy and
>> paste the original Jetty sources of sendError() and modify the
>> contents?
>>
>> Thanks for the attention,
>>
>> Christian
>> BaseX Team
>> _______________________________________________
>> jetty-dev mailing list
>> [hidden email]
>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
> _______________________________________________
> jetty-dev mailing list
> [hidden email]
> https://dev.eclipse.org/mailman/listinfo/jetty-dev
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Greg Wilkins-5
For some reason the gzip filter does not allow gzipped content for
status codes >=300.  I will have to research why.

But it should be able to be made work by making sure you set the
status before you call getOutputStream or getWriter.  The ISE
indicates that one of these methods has been called before the
setStatus, so the filter has decided that it will compress, but then
the setStatus contradicts that.

cheers
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Christian Grün
> But it should be able to be made work by making sure you set the
> status before you call getOutputStream or getWriter.  The ISE
> indicates that one of these methods has been called before the
> setStatus, so the filter has decided that it will compress, but then
> the setStatus contradicts that.

Thanks for the details. True, we are requesting the output stream
reference first in order to attach it to our query processor (which
then throws an error, as the query cannot be evaluated). As this can
hardly be changed in our architecture, as we'd have to extend our
APIs, we'll probably keep gzip compression deactivated for now.

If you think this could be fixed, though.. Even better!
Christian

> _______________________________________________
> jetty-dev mailing list
> [hidden email]
> https://dev.eclipse.org/mailman/listinfo/jetty-dev
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Greg Wilkins-5
I believe we can fix this, but it will not be fixed in jetty-6.

However, the other thing you can try is to call resetBuffer just
before setting the status and then get the outputStream again (which I
think you do anyway).

cheers


On 24 April 2012 23:39, Christian Grün <[hidden email]> wrote:

>> But it should be able to be made work by making sure you set the
>> status before you call getOutputStream or getWriter.  The ISE
>> indicates that one of these methods has been called before the
>> setStatus, so the filter has decided that it will compress, but then
>> the setStatus contradicts that.
>
> Thanks for the details. True, we are requesting the output stream
> reference first in order to attach it to our query processor (which
> then throws an error, as the query cannot be evaluated). As this can
> hardly be changed in our architecture, as we'd have to extend our
> APIs, we'll probably keep gzip compression deactivated for now.
>
> If you think this could be fixed, though.. Even better!
> Christian
>
>> _______________________________________________
>> jetty-dev mailing list
>> [hidden email]
>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
> _______________________________________________
> jetty-dev mailing list
> [hidden email]
> https://dev.eclipse.org/mailman/listinfo/jetty-dev
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Christian Grün
On Wed, Apr 25, 2012 at 12:23 AM, Greg Wilkins <[hidden email]> wrote:
> I believe we can fix this, but it will not be fixed in jetty-6.
>
> However, the other thing you can try is to call resetBuffer just
> before setting the status and then get the outputStream again (which I
> think you do anyway).

..great, thanks for the hint! Resetting the buffer seems to resolve
the issue. I'll keep you updated if I should stumble upon any other
issue.

Christian
___________________________________

> On 24 April 2012 23:39, Christian Grün <[hidden email]> wrote:
>>> But it should be able to be made work by making sure you set the
>>> status before you call getOutputStream or getWriter.  The ISE
>>> indicates that one of these methods has been called before the
>>> setStatus, so the filter has decided that it will compress, but then
>>> the setStatus contradicts that.
>>
>> Thanks for the details. True, we are requesting the output stream
>> reference first in order to attach it to our query processor (which
>> then throws an error, as the query cannot be evaluated). As this can
>> hardly be changed in our architecture, as we'd have to extend our
>> APIs, we'll probably keep gzip compression deactivated for now.
>>
>> If you think this could be fixed, though.. Even better!
>> Christian
>>
>>> _______________________________________________
>>> jetty-dev mailing list
>>> [hidden email]
>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>> _______________________________________________
>> jetty-dev mailing list
>> [hidden email]
>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
> _______________________________________________
> jetty-dev mailing list
> [hidden email]
> https://dev.eclipse.org/mailman/listinfo/jetty-dev
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Thomas Becker
Hi Greg, Christian,

I've written a test to reproduce the errornous behaviour. But it seems
like that with the current GzipFilter code compressedOutputStream is
not assigned before the decision is made to compress and therefore the
doCompress() method is called. So the current code won't throw that
IllegalStateException and instead will just work and do not compress
status codes > 300.

I've found no evident reason why we don't compress status codes > 300.
But it also doesn't do any harm, so I don't think we urgently need to
change that. Let me know if you think we should.

See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=377613 for details.

Cheers,
Thomas

On Wed Apr 25 00:38:57 2012, Christian Grün wrote:

> On Wed, Apr 25, 2012 at 12:23 AM, Greg Wilkins<[hidden email]>  wrote:
>> I believe we can fix this, but it will not be fixed in jetty-6.
>>
>> However, the other thing you can try is to call resetBuffer just
>> before setting the status and then get the outputStream again (which I
>> think you do anyway).
>
> ..great, thanks for the hint! Resetting the buffer seems to resolve
> the issue. I'll keep you updated if I should stumble upon any other
> issue.
>
> Christian
> ___________________________________
>
>> On 24 April 2012 23:39, Christian Grün<[hidden email]>  wrote:
>>>> But it should be able to be made work by making sure you set the
>>>> status before you call getOutputStream or getWriter.  The ISE
>>>> indicates that one of these methods has been called before the
>>>> setStatus, so the filter has decided that it will compress, but then
>>>> the setStatus contradicts that.
>>>
>>> Thanks for the details. True, we are requesting the output stream
>>> reference first in order to attach it to our query processor (which
>>> then throws an error, as the query cannot be evaluated). As this can
>>> hardly be changed in our architecture, as we'd have to extend our
>>> APIs, we'll probably keep gzip compression deactivated for now.
>>>
>>> If you think this could be fixed, though.. Even better!
>>> Christian
>>>
>>>> _______________________________________________
>>>> jetty-dev mailing list
>>>> [hidden email]
>>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>> _______________________________________________
>>> jetty-dev mailing list
>>> [hidden email]
>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>> _______________________________________________
>> jetty-dev mailing list
>> [hidden email]
>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
> _______________________________________________
> jetty-dev mailing list
> [hidden email]
> https://dev.eclipse.org/mailman/listinfo/jetty-dev

--
thomas becker
[hidden email]

http://webtide.com / http://intalio.com
(the folks behind jetty and cometd)
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Christian Grün
Thanks Thomas,

so it may be that this bug, or problem, is indeed related to Jetty 6.
I haven't done any testing with Jetty 7 for myself; if I'll do, I'll
let you know.

Christian
___________________________

On Wed, Apr 25, 2012 at 1:38 PM, Thomas Becker <[hidden email]> wrote:

> Hi Greg, Christian,
>
> I've written a test to reproduce the errornous behaviour. But it seems like
> that with the current GzipFilter code compressedOutputStream is not assigned
> before the decision is made to compress and therefore the doCompress()
> method is called. So the current code won't throw that IllegalStateException
> and instead will just work and do not compress status codes > 300.
>
> I've found no evident reason why we don't compress status codes > 300. But
> it also doesn't do any harm, so I don't think we urgently need to change
> that. Let me know if you think we should.
>
> See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=377613 for details.
>
> Cheers,
> Thomas
>
>
> On Wed Apr 25 00:38:57 2012, Christian Grün wrote:
>>
>> On Wed, Apr 25, 2012 at 12:23 AM, Greg Wilkins<[hidden email]>  wrote:
>>>
>>> I believe we can fix this, but it will not be fixed in jetty-6.
>>>
>>> However, the other thing you can try is to call resetBuffer just
>>> before setting the status and then get the outputStream again (which I
>>> think you do anyway).
>>
>>
>> ..great, thanks for the hint! Resetting the buffer seems to resolve
>> the issue. I'll keep you updated if I should stumble upon any other
>> issue.
>>
>> Christian
>> ___________________________________
>>
>>> On 24 April 2012 23:39, Christian Grün<[hidden email]>  wrote:
>>>>>
>>>>> But it should be able to be made work by making sure you set the
>>>>> status before you call getOutputStream or getWriter.  The ISE
>>>>> indicates that one of these methods has been called before the
>>>>> setStatus, so the filter has decided that it will compress, but then
>>>>> the setStatus contradicts that.
>>>>
>>>>
>>>> Thanks for the details. True, we are requesting the output stream
>>>> reference first in order to attach it to our query processor (which
>>>> then throws an error, as the query cannot be evaluated). As this can
>>>> hardly be changed in our architecture, as we'd have to extend our
>>>> APIs, we'll probably keep gzip compression deactivated for now.
>>>>
>>>> If you think this could be fixed, though.. Even better!
>>>> Christian
>>>>
>>>>> _______________________________________________
>>>>> jetty-dev mailing list
>>>>> [hidden email]
>>>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>>>
>>>> _______________________________________________
>>>> jetty-dev mailing list
>>>> [hidden email]
>>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>>
>>> _______________________________________________
>>> jetty-dev mailing list
>>> [hidden email]
>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>
>> _______________________________________________
>> jetty-dev mailing list
>> [hidden email]
>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>
>
> --
> thomas becker
> [hidden email]
>
> http://webtide.com / http://intalio.com
> (the folks behind jetty and cometd)
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev
Reply | Threaded
Open this post in threaded view
|

Re: [jetty-dev] setStatus() vs. sendError(), custom error feedback

Thomas Becker
You're welcome.

On Wed Apr 25 14:01:05 2012, Christian Grün wrote:

> Thanks Thomas,
>
> so it may be that this bug, or problem, is indeed related to Jetty 6.
> I haven't done any testing with Jetty 7 for myself; if I'll do, I'll
> let you know.
>
> Christian
> ___________________________
>
> On Wed, Apr 25, 2012 at 1:38 PM, Thomas Becker<[hidden email]>  wrote:
>> Hi Greg, Christian,
>>
>> I've written a test to reproduce the errornous behaviour. But it seems like
>> that with the current GzipFilter code compressedOutputStream is not assigned
>> before the decision is made to compress and therefore the doCompress()
>> method is called. So the current code won't throw that IllegalStateException
>> and instead will just work and do not compress status codes>  300.
>>
>> I've found no evident reason why we don't compress status codes>  300. But
>> it also doesn't do any harm, so I don't think we urgently need to change
>> that. Let me know if you think we should.
>>
>> See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=377613 for details.
>>
>> Cheers,
>> Thomas
>>
>>
>> On Wed Apr 25 00:38:57 2012, Christian Grün wrote:
>>>
>>> On Wed, Apr 25, 2012 at 12:23 AM, Greg Wilkins<[hidden email]>    wrote:
>>>>
>>>> I believe we can fix this, but it will not be fixed in jetty-6.
>>>>
>>>> However, the other thing you can try is to call resetBuffer just
>>>> before setting the status and then get the outputStream again (which I
>>>> think you do anyway).
>>>
>>>
>>> ..great, thanks for the hint! Resetting the buffer seems to resolve
>>> the issue. I'll keep you updated if I should stumble upon any other
>>> issue.
>>>
>>> Christian
>>> ___________________________________
>>>
>>>> On 24 April 2012 23:39, Christian Grün<[hidden email]>    wrote:
>>>>>>
>>>>>> But it should be able to be made work by making sure you set the
>>>>>> status before you call getOutputStream or getWriter.  The ISE
>>>>>> indicates that one of these methods has been called before the
>>>>>> setStatus, so the filter has decided that it will compress, but then
>>>>>> the setStatus contradicts that.
>>>>>
>>>>>
>>>>> Thanks for the details. True, we are requesting the output stream
>>>>> reference first in order to attach it to our query processor (which
>>>>> then throws an error, as the query cannot be evaluated). As this can
>>>>> hardly be changed in our architecture, as we'd have to extend our
>>>>> APIs, we'll probably keep gzip compression deactivated for now.
>>>>>
>>>>> If you think this could be fixed, though.. Even better!
>>>>> Christian
>>>>>
>>>>>> _______________________________________________
>>>>>> jetty-dev mailing list
>>>>>> [hidden email]
>>>>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>>>>
>>>>> _______________________________________________
>>>>> jetty-dev mailing list
>>>>> [hidden email]
>>>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>>>
>>>> _______________________________________________
>>>> jetty-dev mailing list
>>>> [hidden email]
>>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>>
>>> _______________________________________________
>>> jetty-dev mailing list
>>> [hidden email]
>>> https://dev.eclipse.org/mailman/listinfo/jetty-dev
>>
>>
>> --
>> thomas becker
>> [hidden email]
>>
>> http://webtide.com / http://intalio.com
>> (the folks behind jetty and cometd)

--
thomas becker
[hidden email]

http://webtide.com / http://intalio.com
(the folks behind jetty and cometd)
_______________________________________________
jetty-dev mailing list
[hidden email]
https://dev.eclipse.org/mailman/listinfo/jetty-dev