tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Costin Manolache" <>
Subject Re: Proposed simplification of CometEvent
Date Mon, 11 Jun 2007 18:47:26 GMT
For a separate opinion:

In the trunk version:
- the '...' and array return seem strange and generate GC ( not a big issue
those days, but still inconsistent with the
rest of tomcat )

- the API seems a bit over-complex - for example, why
setConfiguration(COMMET_BLOCKING) instead of setBlocking() ?
I agree that it less likely to break implementations by adding to the enom
instead of adding methods to the interface - not sure
why it's not an abstract class in the first place.

- please don't call the method configure(), it's commonly used with a
different meaning ( i.e. setting the port or general configuration).
setConnectionMode, etc. And using the enum doesn't sound consistent with
other APIs either.

- see bellow - I don't think I understand the benefits of mixing blocking
and non-blocking in this interface, it is quite confusing.

In the sandbox version:
- sleep() and setTimeout(int) -> why not sleep(int millis) ? I think it's
confusing to have both and the interactions between them, in
particular setTimeout is marked optional ? It makes sense to have
setTimeout() as a general timeout.

- not sure I understand the use case for isReadable()/isWriteable() - when
will this be called ? My understanding was that when
the connection is readable, a callback will be generated with EventType ==
READ. Also it's very confusing to mix the 'blocking' in the
isReadable()/isWriteable() - it would be much cleaner to say that the
connection is always non-blocking, and add a method to switch to blocking
mode ( then use the regular servlet methods maybe ). Using the ComentEvent
for both is IMHO dangerous.

- will sleep() still allow callbacks ( and if not - what will happen with
them )? What's going to happen with callbacks if callback() is not called ?

In general ( both versions):
- it would be great to move it from o.a.catalina to org.apache.comet


On 6/11/07, Remy Maucherat <> wrote:
> Filip Hanik - Dev Lists wrote:
> > Ok, let me see if I can summarize.
> >
> > 1. Whether you write out the stored buffer using the Poller thread, or a
> > Tomcat worker thread (flushed in Http11xxxProcessor) as described below
> > I originally thought of this as async write, as we are simply doing a
> > write with another one of our threads. Originally when we were talking
> > non blocking writes, I was thinking along the lines of non blocking to
> > where the Comet developer had to do that logic, just as he was writing a
> > socket, possibly like (but not suggested) a
> > CometEvent.nonBlockWrite(ByteBuffer).
> >
> > 2. Do we need non blocking? with the methods of isWriteable and the
> > ability to register(OP_WRITE)->event(WRITE), if the number of bytes you
> > write is usually smaller than the socket buffer, chances are that most
> > writes will be non blocking. I would even argue a large majority would
> > be non blocking, and thus the implementation or the complexity thereof
> > would not be needed. And with the ability to do async writes, means I
> > can create my own thread pool/write queue to perform these writes.
> You are writing the opposite thing to the previous email, and we are
> back to "non blocking is useless". The problem is that I understand
> blocking IO as "write this data, and return when it's done". If the
> socket is in blocking mode, any write done by the servlet may block,
> regardless of what isWriteable says. Of course, it's very unlikely,
> which is why Comet in 6.0.x works.
> > 3. isWriteable - simple method, but I don't like that the method in
> > itself performs actions like adding the socket to a poller etc.
> >   Instead isWriteable==true means that you can write on the socket,
> > isWriteable==false you cannot. This method should be able to be invoked
> > as many times as its wanted, and is thread safe and doesn't do anything
> > funky underneath.
> Ok, so you prefer a more complex API (if I follow "just in case it was
> useful"). I started with an API which would expose all operations, and
> looked into removing what was not explicitly useful.
> > 4. isWriteable - I'm also reading in that you are also suggesting that
> > we use this method to declare if we want blocking or non blocking
> writes.
> No. The situation where write could (maybe) block is if the servlet
> writes in a Tomcat thread. Typically, this is the reply-later design,
> using the sleep/callback methods. The isWriteable method is not used,
> since the servlet merely wants (in that common design) to send a
> response as fast as possible, and typically this sort of response is not
> too large and unlikely to cause IO problems. This blocking behavior is
> allowed in that case to avoid forcing the user to put in more complex
> logic to deal with the partial write + event, and is set just for the
> amount of time it takes to perform the write (note that this ).
> >   At this point this method is doing three things:
> >   a) returns true/false if we can write data
> >   b) delegates a socket to the poller to write data and generate a
> > event(WRITE) to the comet processor
> >   c) configures a write to be blocking or non blocking
> >   This is for sure not what I would expect of a "simple API", if simple
> > means less keystrokes than yes, but simple to me also means intuitive
> > and easily understood.
> So you have plenty of methods to do the same thing.
> > Given points 1-4, this is what is going to happen to every single
> developer
> >  I) They are going to use stream.write and event.isWriteable all the
> > time, not realizing what it actually does
> > II) They are going to get confused when they receive an IOException for
> > trying to perform a write, cause they used isWriteable and the socket
> > went into non blocking mode
> If that's what you want to believe ...
> > At this point, this 'simple' API, obviously not so simple, instead it
> > becomes very complicated, as I would almost have to reverse engineer the
> > code to truly understand what it does.
> > It may be simple to you and me, but that is because we are implementing
> it.
> I really don't see what is complex, especially when you look at the code
> the user would write for the simple cases, where you don't even have to
> use any API besides stream.write:
> - reply later
> - wait for read events, and write data in response to it
> The complex case deals with handling incomplete async writes if you
> don't simply drop connection.
> > so what does this mean to 'isReadable'? That I'm automatically
> > registering for a READ event if it returns false? Maybe I don't want a
> > READ event, I just want to see if any data has trickled in. so if I call
> > sleep(), should I then call isReadable() to reregister for the read. how
> > is this simpler than that register/unregister.
> Read events always occur, unless you use sleep/callback. If this is not
> written clearly in the javadocs already, I need to change them.
> > Where does that leave us, well:
> > a) We are almost in sync on the implementation side of it
> Not really, there's a big disconnect in the understanding of non
> blocking vs blocking, and according to you, non blocking is not useful
> (again).
> > b) I believe your API is not intuitive, nor usable, as it simply doesn't
> > reflect what is going on and/or what a programmer wants done
> > c) Your API doesn't become simpler just cause we merge three methods
> > into one -> configure(NON_BLOCK), write(byte[]), register(OP_WRITE)
> > c) The API I propose, you think is overengineered, I think it just makes
> > things much clearer, the fact that it automatically allows for future
> > extension is a side effect rather than design decision
> My API is simpler because the code the user has to write is more
> straightforward and easier to understand. Feel free to write small
> examples to see for yourself.
> > So bottom line is, user will get the same implementation (or very close
> > to what we've talked about), question is what API are they going to get?
> Rémy
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message