tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Costin Manolache" <cos...@gmail.com>
Subject Re: Proposed simplification of CometEvent
Date Thu, 14 Jun 2007 00:02:54 GMT
> >
> >
> > 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 :)



Well, nio is far from perfect - but that's not the point.

Servlets have a very nice blocking mechanism already - it's the servlet API
:-).
The question is why would you need to have a Commet connection blocking.

I think it's very reasonable to add a blocking waitForEvent() to allow
servlets have a
simpler ( but less efficient ) implementation.

Think about utilities that take the event as param - would they need to
check first
if it's blocking or not ? And what would blocking give you in addition to
waitForEvent() -
which is actually better since it allows you to un-block on any event, not
only a specific
read/write.

>
> >
> >>
> >> > - 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.


Ok.


>
> >
> >
> >
> >
> >> > - 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.


What would happen in the blocking case when a different event happens ?
Isn't it the same, if you want to guarantee single-threaded behaviour ?

Well - is there any docs on what is the intended thread model of comet
servlets,
i.e. SingelThreaded or reentrant ? Multiple events can happen at same time
( or very close -
one event soon after a Comet servlet is called, before it returns), so this
is a general problem.


> 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.


I still don't recognize it as NIO :-), and I am not very comfortable with
the
changes between blocking and non-blocking in NIO ( including all the crazy
things that can happen only in the IO thread ).


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



You would still get the BEGIN event I assume.

All you need to poll data async is waitForEvent(), or select() if you like
to emulate nio.

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


In what thread would you get those ( in blocking mode ?)



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.


I agree, but easy to change :-)

   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


As long as it doesn't require you to do special things in the select()
thread, I'm happy :-)


   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 <remm@apache.org> 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Ã(c)my
> >> >>
> >> >>
> ---------------------------------------------------------------------
> >> >> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> >> >> For additional commands, e-mail: dev-help@tomcat.apache.org
> >> >>
> >> >>
> >> >
> >>
> ------------------------------------------------------------------------
> >> >
> >> > 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: dev-unsubscribe@tomcat.apache.org
> >> For additional commands, e-mail: dev-help@tomcat.apache.org
> >>
> >>
> > ------------------------------------------------------------------------
> >
> > 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: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message