Comet, next steps

[Filip Hanik - Dev Lists <de...@hanik.com>]

Gents,
I have played around with the Comet implementation, fixed a few bugs and 
got the initial it working, including async data from both client and 
server.

I wanted to you get your input on moving forward with the following 
features:

1. If CometServlet.read return false, the adapter should call CometServlet.end, not CometServlet.error (easy)
2. If CometServlet.read throws an error, then the adapter should call CometServlet.error (easy)
3. Keep-Alive socket support, when CometServlet.read returns false, don't close the socket, keep alive should still work and we should still be able to process more HTTP requests on that connection. So change the status to comet=false, and process the next request like a standard HTTP request (medium)
4. The ability to close the channel from the server async (medium) - 
   two ways - a) timeout 
              b) call back from a separate thread

And then the following steps
1. Create a user guide for the CometServlet usage
2. Create an example in servlet-examples

Let me know your thoughts, 

Filip




-- 


Filip Hanik

Re: Comet, next steps

[Remy Maucherat <re...@apache.org>]

Filip Hanik - Dev Lists wrote:
>> Unfortunately, it looks like it:
>> +                for (int i=0; i<inputBuffer.activeFilters.length; i++) {
>> +                    //this resets the remaining flag and the content 
>> length on the filter
>> +                    //if we don't do this, then 
>> request.getInputStream.read will return 0
>> +                    if (inputBuffer.activeFilters[i]!=null) 
>> inputBuffer.activeFilters[i].setRequest(request);
>> +                }
>>
>> This is a big hack to avoid content delimitation.
> I see (and saw before) a simple solution to this
> 1. add to interface InputFilter.append(byte[]|ByteBuffer|ByteChunk)
> but I know you would have a field day if I changed an interface, as the 
> checkin would be much larger and I would for sure get a -1,
> at least with this one I had hoped you took a little consideration 
> befored vetoed.

It's that I have the impression you tried something to not have to use 
proper content delimitation, which is a -1. If you chunk your input (or 
use an appropriate content-length, which will be easier while testing 
since chunking by hand is quite annoying), as you are supposed to, I 
think this stuff will work as is (I did a very convincing session tying 
things in a telnet - my servlet was then doing a System.out.println of 
what was read in the read method, and since there was never any writes, 
I did quite a few read calls in a row).

> In terms of reverting my checkin, can we sit on it over the weekend, 
> since the current code enabled my async client currently, and the old 
> code is broken.

We can sit on your commit for the weekend if it makes you happy, that's 
fine.

> We're not in a hurry with the Comet stuff are we.

Hem, why are we not in a hurry ? Most of the time, there's no reason to 
rush, but there's no reason to delay things either.

> With the proposed feature set, Tomcat would excel beyond the existing 
> async implementations in like Jetty or Weblogic, yet it would support 
> the same as they have today.
> So we got a late start, that doesn't mean we can't jump ahead of them, 
> right ;)

I do not see Tomcat as being late: the needs were not well defined until 
recently. And this lame hack job that is Comet (a pathetic excuse to not 
use SIP) will never get any respect from me, that's for sure. I have 
added support for it because at the moment people have decided to stick 
with the HTTP dead end.

Rémy


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Comet, next steps

[Filip Hanik - Dev Lists <de...@hanik.com>]

Remy Maucherat wrote:
> Filip Hanik - Dev Lists wrote:
>>> Actually, I think the server should be the one closing the 
>>> connection. In other cases, since it's a long running request, 
>>> discarding the connection is easier. In HTTP land, the server is 
>>> always the one in control of keepalive.
>> That's correct, however, the current implementation closes the 
>> connection after read returns false, so there is no keepalive 
>> implemented.
>
> Yes, this case is the client signaling the end of the connection. In 
> some other cases, the server can end the connection gracefully, 
> although with these sort of long running connections, this is almost 
> never going to happen.
>
>>>> 4. The ability to close the channel from the server async (medium) 
>>>> -    two ways - a) timeout               b) call back from a 
>>>> separate thread
>>>
>>> This is too complex/risky: you don't know if the socket is still in 
>>> the poller, and destroying it twice or putting back / writing to a 
>>> destroyed socket is fatal.
>> not doing it, means you are setting yourself up for a DOS attack, 
>> since you can run out of connections, all of them being in polling 
>> state.
>> if the socket is in the poller, shouldn't Socket.destroy get it out 
>> of there.
>
> There is still a timeout. I don't see how the DoS risk is any 
> different from the regular scenario. Maybe it's smaller.
>
> No, sockets in a poller must not be destroyed.
>
>>>> And then the following steps
>>>> 1. Create a user guide for the CometServlet usage
>>>> 2. Create an example in servlet-examples
>>>
>>> There is one already ("chat"); it's a bad, but it works more or 
>>> less, and I used it for testing a bit. I also tested input using a 
>>> similar servlet and telnet, and no problem there.
>>>
>>>> Let me know your thoughts, 
>>>
>>> IMO, jumping on something like this is not the way to go.
>>>
>>> I thought about it a little bit more, and I have to veto your 
>>> commit: read will not return 0 (it's the same as it was before: a 
>>> blocking read, so it cannot return 0). I don't understand what your 
>>> intent is with resetting the remaining bytes numbers, etc. Also, 
>>> trying to take care of programming errors in the servlet is 
>>> pointless: similar errors could be made just as well with the 
>>> regular model, entering infinite loops in a similar way.
>> think you need to check the code once more, read is not forever 
>> blocking, CometServlet.read->request.getInputStream().read(buf) 
>> doesn't read the socket inputstream, it reads the inputBuffer, and 
>> since the bug in the code never filled the buffer, so yes, the bug is 
>> fairly obvious. 
>
> Ok, so the bug is fairly obvious, but I do not see it :) I will review 
> your code, but can you revert your commit in the meantime ?
>
>> even worse, if you override the CometServlet.read method, cause you 
>> want the dialog to continue even though you didn't receive a packet, 
>> then this is what happens
>> 1. The poller knows there is data to be read from the IO socket
>> 2. It wakes up, gets a worker thread, thread invokes the CometServlet,
>> 3. CometServlet.read returns true
>> 4. the worker thread adds the socket back to the poller
>> 5. the poller will immediately wake up, as it has data, the same data 
>> in the buffer from before
>
> If the servlet is not reading data in read, there's going to be a 
> problem anyway. If you
>
>> the bug is that Socket.recbb never gets called when its a Comet event.
>
> Well, it's possible, who knows. However, this is a regular read, which 
> will trickle down to the socket, and cause the usual recvbb call. If 
> you are right, then there's indeed something obviously something I am 
> missing.
>
> The input should be exactly the same as before, except in between 
> reading on the socket, the socket goes in a poller.
>
>> I've spent a good two days getting myself familiar with the code, so 
>> this isn't a quick fix of any sort, and what I did, actually made the 
>> code work, and added in a fully functional client and server push 
>> model. You've done a great initial job, and I would ask you to 
>> reconsider your veto given the fact that I didn't just pull the fix 
>> out of my pants, I worked on it in a very detail oriented manner.
>
> Unfortunately, it looks like it:
> +                for (int i=0; i<inputBuffer.activeFilters.length; i++) {
> +                    //this resets the remaining flag and the content 
> length on the filter
> +                    //if we don't do this, then 
> request.getInputStream.read will return 0
> +                    if (inputBuffer.activeFilters[i]!=null) 
> inputBuffer.activeFilters[i].setRequest(request);
> +                }
>
> This is a big hack to avoid content delimitation.
I see (and saw before) a simple solution to this
1. add to interface InputFilter.append(byte[]|ByteBuffer|ByteChunk)
but I know you would have a field day if I changed an interface, as the 
checkin would be much larger and I would for sure get a -1,
at least with this one I had hoped you took a little consideration 
befored vetoed.

In terms of reverting my checkin, can we sit on it over the weekend, 
since the current code enabled my async client currently, and the old 
code is broken.
We're not in a hurry with the Comet stuff are we.

With the proposed feature set, Tomcat would excel beyond the existing 
async implementations in like Jetty or Weblogic, yet it would support 
the same as they have today.
So we got a late start, that doesn't mean we can't jump ahead of them, 
right ;)

Filip
-- 


Filip Hanik

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Comet, next steps

[Remy Maucherat <re...@apache.org>]

Filip Hanik - Dev Lists wrote:
>> Actually, I think the server should be the one closing the connection. 
>> In other cases, since it's a long running request, discarding the 
>> connection is easier. In HTTP land, the server is always the one in 
>> control of keepalive.
> That's correct, however, the current implementation closes the 
> connection after read returns false, so there is no keepalive implemented.

Yes, this case is the client signaling the end of the connection. In 
some other cases, the server can end the connection gracefully, although 
with these sort of long running connections, this is almost never going 
to happen.

>>> 4. The ability to close the channel from the server async (medium) - 
>>>    two ways - a) timeout               b) call back from a separate 
>>> thread
>>
>> This is too complex/risky: you don't know if the socket is still in 
>> the poller, and destroying it twice or putting back / writing to a 
>> destroyed socket is fatal.
> not doing it, means you are setting yourself up for a DOS attack, since 
> you can run out of connections, all of them being in polling state.
> if the socket is in the poller, shouldn't Socket.destroy get it out of 
> there.

There is still a timeout. I don't see how the DoS risk is any different 
from the regular scenario. Maybe it's smaller.

No, sockets in a poller must not be destroyed.

>>> And then the following steps
>>> 1. Create a user guide for the CometServlet usage
>>> 2. Create an example in servlet-examples
>>
>> There is one already ("chat"); it's a bad, but it works more or less, 
>> and I used it for testing a bit. I also tested input using a similar 
>> servlet and telnet, and no problem there.
>>
>>> Let me know your thoughts, 
>>
>> IMO, jumping on something like this is not the way to go.
>>
>> I thought about it a little bit more, and I have to veto your commit: 
>> read will not return 0 (it's the same as it was before: a blocking 
>> read, so it cannot return 0). I don't understand what your intent is 
>> with resetting the remaining bytes numbers, etc. Also, trying to take 
>> care of programming errors in the servlet is pointless: similar errors 
>> could be made just as well with the regular model, entering infinite 
>> loops in a similar way.
> think you need to check the code once more, read is not forever 
> blocking, CometServlet.read->request.getInputStream().read(buf) doesn't 
> read the socket inputstream, it reads the inputBuffer, and since the bug 
> in the code never filled the buffer, so yes, the bug is fairly obvious. 

Ok, so the bug is fairly obvious, but I do not see it :) I will review 
your code, but can you revert your commit in the meantime ?

> even worse, if you override the CometServlet.read method, cause you want 
> the dialog to continue even though you didn't receive a packet, then 
> this is what happens
> 1. The poller knows there is data to be read from the IO socket
> 2. It wakes up, gets a worker thread, thread invokes the CometServlet,
> 3. CometServlet.read returns true
> 4. the worker thread adds the socket back to the poller
> 5. the poller will immediately wake up, as it has data, the same data in 
> the buffer from before

If the servlet is not reading data in read, there's going to be a 
problem anyway. If you

> the bug is that Socket.recbb never gets called when its a Comet event.

Well, it's possible, who knows. However, this is a regular read, which 
will trickle down to the socket, and cause the usual recvbb call. If you 
are right, then there's indeed something obviously something I am missing.

The input should be exactly the same as before, except in between 
reading on the socket, the socket goes in a poller.

> I've spent a good two days getting myself familiar with the code, so 
> this isn't a quick fix of any sort, and what I did, actually made the 
> code work, and added in a fully functional client and server push model. 
> You've done a great initial job, and I would ask you to reconsider your 
> veto given the fact that I didn't just pull the fix out of my pants, I 
> worked on it in a very detail oriented manner.

Unfortunately, it looks like it:
+                for (int i=0; i<inputBuffer.activeFilters.length; i++) {
+                    //this resets the remaining flag and the content 
length on the filter
+                    //if we don't do this, then 
request.getInputStream.read will return 0
+                    if (inputBuffer.activeFilters[i]!=null) 
inputBuffer.activeFilters[i].setRequest(request);
+                }

This is a big hack to avoid content delimitation.

Rémy


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Comet, next steps

[Filip Hanik - Dev Lists <de...@hanik.com>]

Remy Maucherat wrote:
> Filip Hanik - Dev Lists wrote:
>> Gents,
>> I have played around with the Comet implementation, fixed a few bugs 
>> and got the initial it working, including async data from both client 
>> and server.
>>
>> I wanted to you get your input on moving forward with the following 
>> features:
>>
>> 1. If CometServlet.read return false, the adapter should call 
>> CometServlet.end, not CometServlet.error (easy)
>
> This is an EOF on a long running input. I consider it an unexpected 
> end of the communication = an error. This Comet stuff is only useful 
> for pushing back data.
>
>> 2. If CometServlet.read throws an error, then the adapter should call 
>> CometServlet.error (easy)
>
> This is done already.
>
>> 3. Keep-Alive socket support, when CometServlet.read returns false, 
>> don't close the socket, keep alive should still work and we should 
>> still be able to process more HTTP requests on that connection. So 
>> change the status to comet=false, and process the next request like a 
>> standard HTTP request (medium)
>
> Actually, I think the server should be the one closing the connection. 
> In other cases, since it's a long running request, discarding the 
> connection is easier. In HTTP land, the server is always the one in 
> control of keepalive.
That's correct, however, the current implementation closes the 
connection after read returns false, so there is no keepalive implemented.
>
>> 4. The ability to close the channel from the server async (medium) - 
>>    two ways - a) timeout               b) call back from a separate 
>> thread
>
> This is too complex/risky: you don't know if the socket is still in 
> the poller, and destroying it twice or putting back / writing to a 
> destroyed socket is fatal.
not doing it, means you are setting yourself up for a DOS attack, since 
you can run out of connections, all of them being in polling state.
if the socket is in the poller, shouldn't Socket.destroy get it out of 
there.
>
>
>> And then the following steps
>> 1. Create a user guide for the CometServlet usage
>> 2. Create an example in servlet-examples
>
> There is one already ("chat"); it's a bad, but it works more or less, 
> and I used it for testing a bit. I also tested input using a similar 
> servlet and telnet, and no problem there.
>
>> Let me know your thoughts, 
>
> IMO, jumping on something like this is not the way to go.
>
> I thought about it a little bit more, and I have to veto your commit: 
> read will not return 0 (it's the same as it was before: a blocking 
> read, so it cannot return 0). I don't understand what your intent is 
> with resetting the remaining bytes numbers, etc. Also, trying to take 
> care of programming errors in the servlet is pointless: similar errors 
> could be made just as well with the regular model, entering infinite 
> loops in a similar way.
think you need to check the code once more, read is not forever 
blocking, CometServlet.read->request.getInputStream().read(buf) doesn't 
read the socket inputstream, it reads the inputBuffer, and since the bug 
in the code never filled the buffer, so yes, the bug is fairly obvious. 
even worse, if you override the CometServlet.read method, cause you want 
the dialog to continue even though you didn't receive a packet, then 
this is what happens
1. The poller knows there is data to be read from the IO socket
2. It wakes up, gets a worker thread, thread invokes the CometServlet,
3. CometServlet.read returns true
4. the worker thread adds the socket back to the poller
5. the poller will immediately wake up, as it has data, the same data in 
the buffer from before

the bug is that Socket.recbb never gets called when its a Comet event.

I've spent a good two days getting myself familiar with the code, so 
this isn't a quick fix of any sort, and what I did, actually made the 
code work, and added in a fully functional client and server push model. 
You've done a great initial job, and I would ask you to reconsider your 
veto given the fact that I didn't just pull the fix out of my pants, I 
worked on it in a very detail oriented manner.

Filip
-- 


Filip Hanik

Re: Comet, next steps

[Remy Maucherat <re...@apache.org>]

Filip Hanik - Dev Lists wrote:
> Gents,
> I have played around with the Comet implementation, fixed a few bugs and 
> got the initial it working, including async data from both client and 
> server.
> 
> I wanted to you get your input on moving forward with the following 
> features:
> 
> 1. If CometServlet.read return false, the adapter should call CometServlet.end, not CometServlet.error (easy)

This is an EOF on a long running input. I consider it an unexpected end 
of the communication = an error. This Comet stuff is only useful for 
pushing back data.

> 2. If CometServlet.read throws an error, then the adapter should call CometServlet.error (easy)

This is done already.

> 3. Keep-Alive socket support, when CometServlet.read returns false, don't close the socket, keep alive should still work and we should still be able to process more HTTP requests on that connection. So change the status to comet=false, and process the next request like a standard HTTP request (medium)

Actually, I think the server should be the one closing the connection. 
In other cases, since it's a long running request, discarding the 
connection is easier. In HTTP land, the server is always the one in 
control of keepalive.

> 4. The ability to close the channel from the server async (medium) - 
>    two ways - a) timeout 
>               b) call back from a separate thread

This is too complex/risky: you don't know if the socket is still in the 
poller, and destroying it twice or putting back / writing to a destroyed 
socket is fatal.

> And then the following steps
> 1. Create a user guide for the CometServlet usage
> 2. Create an example in servlet-examples

There is one already ("chat"); it's a bad, but it works more or less, 
and I used it for testing a bit. I also tested input using a similar 
servlet and telnet, and no problem there.

> Let me know your thoughts, 

IMO, jumping on something like this is not the way to go.

I thought about it a little bit more, and I have to veto your commit: 
read will not return 0 (it's the same as it was before: a blocking read, 
so it cannot return 0). I don't understand what your intent is with 
resetting the remaining bytes numbers, etc. Also, trying to take care of 
programming errors in the servlet is pointless: similar errors could be 
made just as well with the regular model, entering infinite loops in a 
similar way.

Rémy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org