Re: Proposed simplification of CometEvent

[Filip Hanik - Dev Lists]

Costin Manolache wrote:
On 6/13/07, Filip Hanik - Dev Lists <[EMAIL PROTECTED]> wrote:

Costin Manolache wrote:
> 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 )
yes, its a new language feature, hence it wasn't available in previous
JDKs or Tomcat.
its a vararg, if we don't want it we can switch to arrays, it works the
same way.


Yes, I know it's a vararg and my concern is not that it is not 
available in
all VMs, but
that it creates temp arrays and GC, which is inconsistent with the 
rest of
tomcat which is very careful with creating objects.
Also - it's quite useless in this particular case, printf is a good 
use case
but
that doesn't mean it should be used everywhere.
true.



> - 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.
if we want to simplify it, we would call it
configureBlocking(boolean) - consistent with JDK APIs :)


Sounds better - but as Remy explained you would first need to explain
why blocking is needed in this context and how to deal with the confusion
of mixing blocking and non-blocking for users, and the implementation
complexities it adds.
trunk doesn't mix them. a comet connection is either blocking or non 
blocking, it doesn't shift between the two,
and it allows developers to choose what they want. Just like a  
SocketChannel in java.nio.
there is nothing confusing about that, unless java.nio is confusing :)




> - 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.
we can call it whatever we want. But saying not using enum, its not
consistent with other APIs in Tomcat,
means would never take advantage of new language features ever, I think
that would be a shame.


Same as above - the question is not about using new features, but if the
features
fit the use. I have no problem with using enums for the event types - 
just
for
configure, in the context of configure(enum) versus setBlocking(), 
setFoo().
this has been adjusted based on the feedback, the method is now 
configureBlocking(boolean)
the state of it can be used by calling isBlocking()

register is using enums, mainly cause Remy, while he was working with 
this API insisted on it.
I had preferred using an int, just like the socket API, but since Remy 
had initially agreed to register, and proposed enum and unregister
we went with that.





> - see bellow - I don't think I understand the benefits of mixing
blocking
> and non-blocking in this interface, it is quite confusing.
It would be mixing it, its a one time config, during the BEGIN event,
you say
configureBlocking(true) or configureBlocking(false).
Comet is very much connection centric, so you can't mix it.

In the trunk API, its clear to what you are using, blocking or non
blocking, in the sandbox API, the swap
of it happens when invoking isWriteable or isReadable, making the state
of the comet connection confusing to the developer.


I'm not sure it's true - my understanding is that sandbox is all
non-blocking.
Invoking isWriteable is not blocking.

I think it would be ok to add a blocking waitForEvent() - combined with
isReadable()/isWriteable()
this would be a dead lock, as the Comet API must guarantee that a 
CometProcessor.event
is only invoked by one worker thread at any time. The blocking you are 
talking about can be done
using an async thread can be done by registering for the event you wish 
to be notified in and then
maybe await a latch countdown, or doing a sync/wait() combo.

it will allow a sort-of blocking model. But having methods that are 
blocking
sometimes
and sometimes not - based on some config - is dangerous and confusing.
As I mentioned, the trunk Comet API is actually very similar to java.nio 
API, hence developers familiar with that will instantly recognize
themselves and should get comfortable with the API.

Here are some of the things that might need to be clarified about the 
trunk API:

1. It is possible to not be registered for any events, and even do 
polling for data asynchronously
2. ERROR and END events, are an exception, you cant unregister for 
those, as those might require the
  underlying socket to be closed, or has already been closed
3. I believe it to be much cleaner to follow an event model, after all 
that is what Comet is.
4. In previous emails I have questioned the usability of "non 
blocking/async" reads and writes,
  as they are not implemented to true non block unless they entire 
buffer/filter stack is modified.
  A skilled Comet developer could take advantage of isReadable() or 
register(READ) and register(WRITE)
  to accomplish that exact thing without us having to implement the 
complexity of non blockable code,
  without rewriting lots of the coyote stuff.
5. The current READ event in CoyoteAdapter ends up performing a blocking 
read if data has been received but its not enough
  for ServletInputStream.read() to actually generate data, just so that 
it can produce a real READ event,
  there is no real way to get around this without a major effort 
described in 4)
6. sleep() and callback() are names that already have confused folks in 
this thread, they just don't make sense.
  But its not the names alone, people think of Comet as 
request/response stuff, when it isn't.
  Comet is centered around a socket connection, and the events 
generated are a direct derivative of that.
  That is why I have chosen to make the API similar to what's been in 
Java since 1.4
  Comet is definitely more advanced than a servlet, and the complexity 
of it is higher,
  having less methods on an API wont make it less complex, since the 
complexity sits in understanding the concept.


I'm gonna work up some examples so that we can take a look at different 
scenarios and how the APIs would differ

Filip




Costin

Filip
>
> 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
>
> Costin
>
> On 6/11/07, Remy Maucherat <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
>> For additional commands, e-mail: [EMAIL PROTECTED]
>>
>>
> 
------------------------------------------------------------------------
>
> Internal Virus Database is out-of-date.
> Checked by AVG Free Edition.
> Version: 7.5.472 / Virus Database: 269.8.11/837 - Release Date: 
6/6/2007
2:03 PM
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]


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

No virus found in this incoming message.
Checked by AVG Free Edition. 
Version: 7.5.472 / Virus Database: 269.8.15/848 - Release Date: 6/13/2007 12:50 PM
  


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]