Bug 50747

Summary: CometProcessor does not flush and close HTTP/1.0 requests
Product: Tomcat 7 Reporter: Frank Schroeder <frank.schroeder>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED DUPLICATE    
Severity: normal    
Priority: P2    
Version: 7.0.8   
Target Milestone: ---   
Hardware: PC   
OS: All   

Description Frank Schroeder 2011-02-09 14:16:29 UTC 
I have built a simple Publish/Subscribe servlet using the CometProcessor. When a client connects then the servlet checks in the BEGIN phase whether there are pending events for the client and if not stores the request in a list of pending requests. Later another thread notifies the CometProcessor that data for the user has arrived and the data is pushed through the pending connection. 

The pseudo-code looks like this:

public class EventService implements CometProcessor {
    public void event(CometEvent event) {
        if (event.getType() == CometEvent.BEGIN) {
            String data = getDataForUser(event);
            if (data != null) {
                sendAndClose(data, event);
            } else {
                pendingRequests.add(event);
            }
        } else {
            event.close();
        }
    }

    public void push(String user, String data) {
        CometEvent event = findPendingRequest(user);
        sendAndClose(data, event);
        pendingRequests.remove(event);
    }

    public void sendAndClose(String data, CometEvent event) {
        Writer w = event.request.getWriter();
        w.write(data);
        w.flush();
        event.close();
    }
}

This works as expected as long as I connect with an HTTP/1.1 client. However, when an HTTP/1.0 client connects (e.g. nginx) the connection is not closed immediately. In my case the data is a JSON string which is pushed to the client if the client ends with '\r\n' but the connection lingers open. 

This is also reproducible with curl

HTTP/1.1 request
----------------

# curl -v -XGET -u 'frank:frank' 'http://127.0.0.1:8081/fcc/event?timestamp=1297277309368'; echo 
> GET /fcc/event?timestamp=1297277309368 HTTP/1.1
> Authorization: Basic xxx
> User-Agent: curl/7.21.2 ...
> Host: 127.0.0.1:8081
> Accept: */*
> 
< HTTP/1.1 200 OK
< Server: Apache-Coyote/1.1
< Content-Type: application/json;charset=ISO-8859-1
< Transfer-Encoding: chunked
< Date: Wed, 09 Feb 2011 18:49:45 GMT
< 
{"type":"settings-changed","timestamp":1297277385366,"data":{"version":157}}
* Connection #0 to host 127.0.0.1 left intact
* Closing connection #0
^^^^^^^^^^^^^^^^^^^^^^^
Connection is closed immediately

HTTP/1.0 request
----------------

# curl -v -XGET -0  -u 'frank:xxx' 'http://127.0.0.1:8081/fcc/event?timestamp=1297277309368'; echo 
> GET /fcc/event?timestamp=1297277309368 HTTP/1.0
> Authorization: Basic xxx
> User-Agent: curl/7.21.2 ...
> Host: 127.0.0.1:8081
> Accept: */*
> 
< HTTP/1.1 200 OK
< Server: Apache-Coyote/1.1
< Content-Type: application/json;charset=ISO-8859-1
< Date: Wed, 09 Feb 2011 18:49:45 GMT
< Connection: close
< 
{"type":"settings-changed","timestamp":1297277385366,"data":{"version":157}}
^^^^^^^^^^^^^^^^^^^^^^^
Connection stays open until timeout occurs
Comment 1 Frank Schroeder 2011-02-09 15:21:54 UTC 
I've found the problem. The following sequence works with HTTP/1.1 but not with HTTP/1.0. I guess I've made a classical optimization mistake. This also explains why the content length and encoding headers were never set.


String data = "...";
PrintWriter writer = response.getWriter();
response.setStatus(200);
response.setCharacterEncoding("UTF-8");
response.setContentLength(data.length());
writer.write(data);
writer.flush();
event.close();

Getting the Writer *after* setting the response headers makes it all work

String data = "...";
response.setStatus(200);
response.setCharacterEncoding("UTF-8");
response.setContentLength(data.length());
PrintWriter writer = response.getWriter();
writer.write(data);
writer.flush();
event.close();
Comment 2 Konstantin Kolinko 2011-02-09 15:39:00 UTC 
> String data = "...";
> response.setCharacterEncoding("UTF-8");
> response.setContentLength(data.length());
> PrintWriter writer = response.getWriter();

The above code is wrong, because String.length() is measured in chars, but content-length header is measured in bytes. UTF-8 uses more than 1 byte for characters > 127.
Comment 3 Mark Thomas 2011-02-09 15:48:04 UTC 
Tomcat could be a little smarter here.

We currently ignore a call to setContentLength() after a call to getWriter(). However, we only have to ignore the content length once bytes have actually been written to the response and a method to determine that is available.

If I were to patch trunk, could you build 7.0.x from source and give it a try?
Comment 4 Frank Schroeder 2011-02-09 15:55:29 UTC 
Sure, I can try that. While you're at it you can also check the encoding as it wasn't set as well. The content type was OK. The reason I had this code was because of this:

PrintWriter writer = response.getWriter();
if (jsonp) {
String data = ... build jsonp here ...
  response.setStatus(200);
  response.setContentType("text/javascript");
  response.setCharacterEncoding("UTF-8");
  response.setContentLength(data.length());
  writer.write(data);
} else {
  String data = ... build json here ...
  response.setStatus(200);
  response.setContentType("application/json");
  response.setCharacterEncoding("UTF-8");
  response.setContentLength(data.length());
  writer.write(data);
}
writer.flush();
event.close();


Regarding the content length. So I guess that should be then like this?

response.setContentLength(data.getBytes("UTF-8").length);

As a side node and for completeness: 

nginx still hung but turning off proxy buffering with

proxy_buffering off;

fixed that as well.
Comment 5 Mark Thomas 2011-02-09 16:16:04 UTC 
You can't change the encoding once the writer has been obtained since the encoding is used to create the writer.
Comment 6 Konstantin Kolinko 2011-02-09 16:54:08 UTC 
(In reply to comment #3)
> Tomcat could be a little smarter here.

For reference: I filed bug 50748 to deal with setContentLength() improvements.


(In reply to comment #4)
> Regarding the content length. So I guess that should be then like this?
> response.setContentLength(data.getBytes("UTF-8").length);
> 

If you already called getBytes() then use byte[] array that it returns and pass to an OutputStream. You won't need a Writer. There is no need to do the double work.
Comment 7 Mark Thomas 2011-02-09 17:19:51 UTC 
This issue is part useful discussion, part bug.

The bug part has moved to 50748 and the discussion should move to the users mailing list.

*** This bug has been marked as a duplicate of bug 50748 ***
Comment 8 Frank Schroeder 2011-02-10 03:03:12 UTC 
Just for the sake of completeness: I've changed the code to

String data = "...";
byte[] buf = data.getBytes(charset);
response.setContentLength(buf.length);
response.getOutputStream().write(buf);
response.getOutputStream().flush();
cometEvent.close();