On 6/14/07, Filip Hanik - Dev Lists <[EMAIL PROTECTED]> wrote:
Costin Manolache wrote: >> > >> > >> > 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.
because comet is not a servlet, and the ease of use for having blocking
write and read is huge, especially when Tomcat can notify you when you can write or read, there is no need to scratch your head and try to use non blocking. non blocking is more complex, and not for everyone.
I'm not sure I understand - there is a perfectly fine blocking read/write interface, it's the plain servlet API. I also agree that 'blocking' mode is sometimes easier to code than event-based, and I can see the benefit of doing some stuff in blocking mode and some in non-blocking. My concern was making the read and write blocking in a commet servlet based on a config in the CometEvent. The alternative is to have the comet servlet allways non-blocking for read/write, but provide a convenience method that will simulate the blocking read or write ( which is easy, all you need is a blocking waitForEvent(time) ). Benefits: - the read/write implementation is simpler ( no need to check the config mode or do tricks ), - comet is easier to understand - you can do more advanced things, like using reads in non-blocking mode and writes in blocking mode. The code using waitForEvent() is a bit more complicated than the code for blocking read/write ( but if you want the simplest solution - use regular servlet ), and it is simpler than a pure callback based model.
> I think it's very reasonable to add a blocking waitForEvent() to allow > servlets have a > simpler ( but less efficient ) implementation.
Comet inherently is a "wait for event" system. Tomcat acts as a
multiplexer/dispatcher,and fires events into the CometProcessor.
Comet seems to be an event-based system, that's why setting it to 'blocking mode' is so confusing and bad. I'm not sure I agree it has to be a 'blocking wait for event' system - in Remy examples at least it is not. The "waitForEvent()" method is obsolete in the Comet implementation, as
you are either on a Tomcat thread, which you can activate at any time or you are on a async thread, and not sure why you would need to block an async background thread. > > 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. I can see that there is a huge misunderstanding of how comet actually does work and what it actually is. It is true that is essentially just a TCP socket between server and client, but in addition to that, Tomcat provides a shell around it, and instead of you having to manage when to read or when to write, tomcat becomes an event API.
Sure, it hides some limitations of NIO ( the IO thread that has to do all things ) and adds some higher-level HTTP stuff.
>> >> > - 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. I still think an int is better :) > > >> >> > >> > >> > >> > >> >> > - 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, A comet servlet is not single-threaded, but a comet event is. from Tomcat's stand point. furthermore, a comet event (which represent a socket connection) can be used multithreaded, but it will be the developers responsibility to sync appropriately.
Ok, it makes sense. But I assume the comet event is not imutable - i.e. you get the event, and its state can change ( isReadable() will change, it can represent another state, etc ).
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 ). exactly, and that is why in Tomcat you have the ability to truly rely on the Tomcat worker threads to get your work done. That way you never have to sync or worry about stuff like that Albeit there will be programmers that need more, and for those we can offer more. > > > 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. Yes, and events are always sent on a Tomcat thread, and events are guaranteed to be one-thread-per-comet-event
> All you need to poll data async is waitForEvent(), or select() if you > like > to emulate nio. you don't even need that. this is an event API, register for the events, and they will happen.
Sure - all I was saying is that you could get most of the blocking programming model by using a waitForEvent() instead of setting the socket in blocking mode, just like NIO selects() ( of course, nio select must be in the io thread- and comet will hide this ).
> 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 ?) Tomcat thread, > > > > 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 :-) The comet programmer never has to worry about a select() thread, that logic is already taken care of for you, and working in 6.0.13
Thanks for the extra info, I think I understand much better now the 2 comets. I would still vote for the sandbox version and a pure event system ( with emulated blocking if needed, via a waitForEvent() mechanism ). In any case - the 2 seem close enough and could be easily merged, at least at API level, but I hope the merge will not keep the blocking configuration. Costin Filip
> > > 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.isWriteableall >> >> 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: [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] >> >> > ------------------------------------------------------------------------ > > 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]
