[Geoserver-devel] [geoserver-scm] [10383] trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java: Don't use .available() to check whether streams have data in them.

GeoServer GeoServer Developer
1

Hi David,

I am a little concerned about this patch. I don't have a concrete -1 against it but I remember that check handling various cases and an issue arising when I once tried to take it out as well.

Andrea might have a better idea of what that issue actually was. But just a word of caution to proceed carefully.

It might be good for us to add a few more test cases to the dispatcher tests testing some of the more obscure cases. Such as simulating input from a form... or sending both content in the body and parameters as kvp's.

-Justin

dwinslow@anonymised.com wrote:

Revision
    10383 <http://fisheye.codehaus.org/changelog/geoserver/?cs=10383&gt;
Author
    dwinslow
Date
    2008-10-13 17:21:06 -0500 (Mon, 13 Oct 2008)

      Log Message

Don't use .available() to check whether streams have data in them. Fixes GEOS-2281 <http://jira.codehaus.org/secure/ViewIssue.jspa?key=GEOS-2281&gt; (log filter killing POST requests when body logging enabled)

      Modified Paths

    * trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java
      <#trunkgeoserverowssrcmainjavaorggeoserverowsDispatcherjava>

      Diff

        Modified:
        trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java
        (10382 => 10383)

--- trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java 2008-10-13 19:03:31 UTC (rev 10382)
+++ trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java 2008-10-13 22:21:06 UTC (rev 10383)
@@ -223,17 +223,19 @@
         //create the kvp map
         parseKVP(request);
         - if ( !request.get && httpRequest.getInputStream().available() > 0) {
- //wrap the input stream in a buffer input stream
+ if ( !request.get ) { // && httpRequest.getInputStream().available() > 0) {
+ //wrap the input stream in a buffered input stream
             request.input = reader(httpRequest);
- //mark the input stream, support up to 2KB, TODO: make this configuratable
+ //mark the input stream, support up to 2KB, TODO: make this configurable
             request.input.mark(2048);
                          if (logger.isLoggable(Level.FINE)) {
                 char req = new char[1024];
                 int read = request.input.read(req, 0, 1024);
- if (read < 1024) {
+ if (read == -1) {
+ request.input = null;
+ } else if (read < 1024) {
                     logger.fine("Raw XML request starts with: " + new String(req));
                 } else {
                     logger.fine("Raw XML request starts with: " + new String(req) + "...");

------------------------------------------------------------------------

To unsubscribe from this list please visit:

http://xircles.codehaus.org/manage_email

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

2

If this is a real concern then we can do the following:
* roll back the aforementioned change
* tweak the logging filter to do a read and a rewind on the stream that it's returning to ensure that available() returns something bigger than zero
* increase test coverage and do it this way (I'd argue the 'right' way) for 1.7.1

That said, stream.available() == 0 does not imply that the stream has no contents, just that 0 bytes can be read without blocking. Relying on it to return >0 for non-empty streams will cause problems in some situations such as this one, so ultimately I'd like to see the .available()-less implementation used here. For what it's worth, I haven't seen any issues since making this change; but I also have not made a point of trying out the different edge cases wrt POST/GET parameter mixing.

-David Winslow

Justin Deoliveira wrote:

Hi David,

I am a little concerned about this patch. I don't have a concrete -1 against it but I remember that check handling various cases and an issue arising when I once tried to take it out as well.

Andrea might have a better idea of what that issue actually was. But just a word of caution to proceed carefully.

It might be good for us to add a few more test cases to the dispatcher tests testing some of the more obscure cases. Such as simulating input from a form... or sending both content in the body and parameters as kvp's.

-Justin

dwinslow@anonymised.com wrote:
  

Revision
    10383 <http://fisheye.codehaus.org/changelog/geoserver/?cs=10383&gt;
Author
    dwinslow
Date
    2008-10-13 17:21:06 -0500 (Mon, 13 Oct 2008)

      Log Message

Don't use .available() to check whether streams have data in them. Fixes GEOS-2281 <http://jira.codehaus.org/secure/ViewIssue.jspa?key=GEOS-2281&gt; (log filter killing POST requests when body logging enabled)

      Modified Paths

    * trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java
      <#trunkgeoserverowssrcmainjavaorggeoserverowsDispatcherjava>

      Diff

        Modified:
        trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java
        (10382 => 10383)

--- trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java 2008-10-13 19:03:31 UTC (rev 10382)
+++ trunk/geoserver/ows/src/main/java/org/geoserver/ows/Dispatcher.java 2008-10-13 22:21:06 UTC (rev 10383)
@@ -223,17 +223,19 @@
         //create the kvp map
         parseKVP(request);
         - if ( !request.get && httpRequest.getInputStream().available() > 0) {
- //wrap the input stream in a buffer input stream
+ if ( !request.get ) { // && httpRequest.getInputStream().available() > 0) {
+ //wrap the input stream in a buffered input stream
             request.input = reader(httpRequest);
- //mark the input stream, support up to 2KB, TODO: make this configuratable
+ //mark the input stream, support up to 2KB, TODO: make this configurable
             request.input.mark(2048);
                          if (logger.isLoggable(Level.FINE)) {
                 char req = new char[1024];
                 int read = request.input.read(req, 0, 1024);
- if (read < 1024) {
+ if (read == -1) {
+ request.input = null;
+ } else if (read < 1024) {
                     logger.fine("Raw XML request starts with: " + new String(req));
                 } else {
                     logger.fine("Raw XML request starts with: " + new String(req) + "...");

------------------------------------------------------------------------

To unsubscribe from this list please visit:

http://xircles.codehaus.org/manage_email

3

David Winslow wrote:

If this is a real concern then we can do the following:
* roll back the aforementioned change
* tweak the logging filter to do a read and a rewind on the stream that it's returning to ensure that available() returns something bigger than zero
* increase test coverage and do it this way (I'd argue the 'right' way) for 1.7.1

+1. I would say go ahead with the change on trunk. And lets write some test cases for the dispatcher testing some of the corner cases. And when we feel more confident we can commit on 1.7.x for 1.7.1.

That said, stream.available() == 0 does not imply that the stream has no contents, just that 0 bytes can be read without blocking. Relying on it to return >0 for non-empty streams will cause problems in some situations such as this one, so ultimately I'd like to see the .available()-less implementation used here. For what it's worth, I haven't seen any issues since making this change; but I also have not made a point of trying out the different edge cases wrt POST/GET parameter mixing.

I agree, the check is not 100% robust, but it was put in to fix a particular use case so I fear changing it will introduce a regression. But I realize that because I can't remember what the case is I am not making a very strong case.

4

Okay, we'll go with this then. I'll take care of the revert on 1.7.x before I call it quits for today.

-David

Justin Deoliveira wrote:

David Winslow wrote:
  

If this is a real concern then we can do the following:
* roll back the aforementioned change
* tweak the logging filter to do a read and a rewind on the stream that it's returning to ensure that available() returns something bigger than zero
* increase test coverage and do it this way (I'd argue the 'right' way) for 1.7.1
    

+1. I would say go ahead with the change on trunk. And lets write some test cases for the dispatcher testing some of the corner cases. And when we feel more confident we can commit on 1.7.x for 1.7.1.
  

That said, stream.available() == 0 does not imply that the stream has no contents, just that 0 bytes can be read without blocking. Relying on it to return >0 for non-empty streams will cause problems in some situations such as this one, so ultimately I'd like to see the .available()-less implementation used here. For what it's worth, I haven't seen any issues since making this change; but I also have not made a point of trying out the different edge cases wrt POST/GET parameter mixing.

I agree, the check is not 100% robust, but it was put in to fix a particular use case so I fear changing it will introduce a regression. But I realize that because I can't remember what the case is I am not making a very strong case.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel