tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Hanik - Dev Lists <devli...@hanik.com>
Subject Re: Proposed simplification of CometEvent
Date Fri, 08 Jun 2007 12:41:26 GMT
Remy Maucherat wrote:
> Filip Hanik - Dev Lists wrote:
>> If you don't care about this stuff, but still read the first line, do 
>> our community a favor and get involved :)
>
> I'm not sure I understand this. It could be interpreted in a 
> derogatory way.
if it is just you and I, then its your -1 against my -1 and we get nowhere.
simply trying to invite folks to be a part of the decision making.
>
>> The API's are pretty much identical,
>
> Yes, they are pretty much functionally identical. The problem is that 
> the interpretation of the same thing you are proposing is 
> overengineered, convoluted and very GC unfriendly. The sync was a 
> funny choice which made me start working on a separate branch, since:
> - you were ignoring my messages all of a sudden for some reason
no messages ignored. some messages were pure statements, and didn't 
invite for comments.
> - you started adding more and more stuff I disagreed with
to the API? (CometEvent.java) like what?
> It's ok to do that as part of a proposal (like I did), but it's not 
> the right way if you do it in a regular branch.
>
>> callback()
>> - Remy - one time callback from the container on a Tomcat worker thread
>> - Filip - same functionality implemented, 
>> CometEvent.register(OP_CALLBACK)
>> I definitely like the register/unregister for events more than I like 
>> the one time functionality of callback()/sleep().
>> OP_CALLBACK currently is implemented and working in trunk.
>> Why do I like register more? Cause we can add new operations in the 
>> future without changing the API. This leads to more flexibility.
>> Currently OP_CALLBACK stays registered until it is "unregister"-ed. 
>> If we wanted to achieve callback() functionality, we can add a 
>> OP_CALLBACK_ONCE operation or simply change the behavior of existing 
>> callback, without changing the API.
>> it is also possible to retrieve the state of a comet connection by 
>> calling getRegisteredOps, something not available in the sandbox API.
>>
>> sleep()
>> - Remy - deregister from the READ events
>> - Filip - same functionality exists, 
>> CometEvent.unregister(OP_READ,OP_WRITE,OP_CALLBACK) or simply 
>> CometEvent.unregister(OP_READ) if nothing else is registered.
>> Again, I think the trunk API is more flexible, and cleaner. sleep() 
>> is associated with Thread.sleep() in every programmers head. and 
>> doesn't really apply to an event based API.
>> This is implemented in trunk and working.
>
> The connector should only care about IO, which only knows read and 
> write (and error, but obviously this is a special case, and I don't 
> see you registering for that).
But the connector must care, if you do a CALLBACK, you must ensure that 
you are not spawning a 2nd thread if a READ comes in on the socket.
otherwise, you are forcing the comet developers to synchronize, 
something in today's API, is done in such a way that if you do stuff on 
the Tomcat thread, you're safe.
>
>> Conclusion, both APIs are almost identical. I believe the trunk API 
>> is more flexible and allows for additions of functionality in the 
>> future without changing the API.
>> Remy's initial -1 to the trunk was based on the implementation, 
>> having to synchronize etc. All that has been simplified and removed.
>
> I would like to understand what extensions are desirable or 
> envisioned, since this this sort of extensions will make the connector 
> implementation to skyrocket in terms of complexity. I think the 
> connector should only care about IO, so this means: read, write, and a 
> generic callback. The user will write a piece of code which can listen 
> to any sort of event, and use the callback method: the callback is the 
> extension point as far as I understand.
>
> Another flaw IMO is that read and write do not behave the same as far 
> as their "registration" go.
>
> The trunk API also has the following additional methods and constructs:
> - public enum CometConfiguration {COMET_BLOCKING, COMET_NON_BLOCKING};
> - public void configure(CometConfiguration... options) throws 
> IllegalStateException; (note: as I have discussed, it seems possible 
> to abstract away blocking configuration from the user)
> - public enum CometOperation {OP_CALLBACK, OP_READ, OP_WRITE};
> - public CometOperation[] getRegisteredOps();
> - the need to ask for a write event (it is quite crucial, and I found 
> out it could easily be hidden away)
There is no need to ask for a WRITE, you can write anytime, 
asynchronously too. OP_WRITE is if you want to be sure that the network 
buffer is ready to receive data.
otherwise, I already pointed out the flaws in isWriteable and that 
isWriteable==true doesn't mean that it is writeable.
> So, similarly, the configuration is extensible. I would like to 
> understand what additional configuration should be exposed to the user 
> (personally, I solved the configuration problem when I noticed 
> blocking / non blocking configuration was pointless).
configure() can be removed, or taken out for a later consideration, have 
no problem with that.
>
> I also dislike the thread checks you are using.
I can pull those out as well, since in my impl I able to check the 
"stage" of the request, then I don't need to know what thread is doing what.
>
> The last reason I would like to have a really simple (yet fully 
> functional) API is to avoid having duplicate APIs. The Servlet API may 
> tackle the Comet topic, and knowing Sun APIs, you may get the API of 
> your dreams :D
once the Servlet API does tackle it, we'll deal with it then.
>
>> So therefor, the API's are almost identical, I would vote
>> sandbox API -> -1 -> API is simple but not extensible, names are not 
>> intuitive as they are not event oriented.
>> trunk API -> +1 -> API is equally simple, more geared towards event 
>> based API, more flexible, as I am sure new needs will need new 
>> operations and events
>
> I think the APIs are quite different in form, yet expose similar 
> functionality (at the moment, I'm waiting for the OP_JMS ;) ). Since 
> it's a user visible API, it's an important issue regardless of what 
> you think. I have my own vote (hehe ;) ):
> - sandbox -> +1
> - "trunk" -> -1 (overenginneered, more complex, thread checks)
exactly my point, we're getting nowhere.
>
>> The only thing not working in trunk at this moment is non blocking, 
>> but after doing the research, this is a major surgery to try to do 
>> within the existing buffer/filter API
>
> Actually, this is very simple to implement. Read is obvious. For 
> write, the algorithm for the flushBuffer method would be very similar 
> to what it is now, but if a write returns 0, leftover bytes would be 
> put in a ByteChunk. isWriteable, if called, will return false, and 
> place the socket in the poller with write notifications.
that's not non blocking, that's a async write, and I could ten times 
more easier implement it in an AbstractCometProcessor.java, then having 
to fiddle with pollers and everything else down the chain.
non blocking is a different concept than async.
>
> There are two options at that point:
>
> - the servlet does not use isWriteable (so does not care about 
> possible congestion when writing): on the next write, the leftover 
> bytes are flushed (most likely the additional bytes which are sent 
> would be added to the ByteChunk, to attempt to maximize the amount of 
> bytes written - in case somehow it works), and if writing still fails 
> an IO exception is thrown if the write was called asynchronously; if 
> the write was synchronous (= called from a Tomcat thread), it is 
> possible for ease of use to make another write, but switch to blocking 
> mode
>
> - the servlet uses isWriteable: no problem, it will get false, stop 
> writing immediately (unless the guy did not understand the API, lol), 
> and a write event will be eventually generated by the poller; when 
> this write occurs, the leftover bytes - if any - will be flushed (if 
> somehow there are leftovers, then it's "go back to the poller, you 
> lazy stupid connection !"), isWriteable will start returning true, and 
> a write event will be sent to the servlet
does not work either. isWriteable (when blocking) only is in effect for 
other threads, but write is not thread safe, so this method is useless then.
and during an async write, you have the same issue, async writes still 
should only be done on one thread.

In my personal opinion I am no longer sure a non blocking write is needed.
>
>> I'd rather delay such an attempt until we can reach consensus on the 
>> API and the "simple stuff".
>
> I agree a consensus should have been reached *on month ago* :)
we'd be in the same shoes we are today. I still think a pancake flipping 
contest would be the way to go. although I might consider stone,paper 
and scissors too.
Filip

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


Mime
View raw message