Any comments on this niklas ? Wdyt about it ?
Bye,
Norman
2010/12/15 Norman Maurer <nor...@apache.org>:
> Hi Niklas,
>
> comments inside :)
>
>
> 2010/12/15 Niklas Therning <nik...@trillian.se>:
>> Hi,
>>
>> The synchronized block is actually there to prevent concurrent access to the
>> responder object. I realize that may not be very clear. :-) Initially I had
>> a synchronized block around the whole
>>
>> if (!"DONE".equals(...)) {
>> ...
>> } else {
>> ...
>> }
>>
>> block but then I realized that it wasn't necessary. Please let me know if
>> you think differently.
>>
>
> Ah ok I missed this before, anyway then you could replace the
> AtomicBoolean with an boolean.. But let us not start nit-picking atm
> ;)
>
>
>> Anyways, maybe I should explain the code a bit...
>>
>> I've added IdleCommandParser and IdleRequest which handle the new IDLE
>> command. This was pretty much just copy paste from the NoopCommandParser and
>> NoopRequest.
>>
>> The IdleProcessor sends back a continuation request response (+ Idling) to
>> the client, registers a listener on the current mailbox and then waits for
>> the client to send DONE. When the listener gets notified of any changes to
>> the messages in the mailbox (new messages, deleted messages or flags
>> changed) it will write the same kind of untagged response back to the client
>> as NOOP does. Once the client sends DONE (or anything really) the
>> IdleProcessor will remove the listener and send back a tagged response to
>> the client which terminates the IDLE command.
>>
>> One thing that I had to add was support for writing continuation request
>> responses to the client (+ Yadayada) and then handle the next line the
>> client sends to the server. The ContinuationReader interface is used to read
>> a line from the client and the processor will have to parse it and handle
>> it. This line will not be parsed by the decoder which usually parses
>> commands sent by the client. I don't know if this is a good approach but it
>> seems to work and I think it could be used to implement AUTHENTICATE as
>> well.
>>
>
> Ok I see... To be honest I think the current implementation of the
> line handling is not good, which is not related to what you did. I'm
> more talking about how its handled in the whole implementation. I
> think to make it a good fit for usage in most NIO-Frameworks (like
> MINA or Netty) we should refactor it to follow a "push" model.
> Something like we did in the protocols-smtp code.
>
> Like have an interface like this one:
>
> http://svn.apache.org/viewvc/james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/LineHandler.java?view=markup
>
> which would get "triggered" on every received line (terminated by
> CRLF). We would also need to support some kind of pushing an other
> LineHandler implementation in for some time to handle the STORE
> command in good way. We already did something like this in our smtp
> code to allow to "push" a new LineHandler in to handle message
> received after the DATA command.
>
> Something like this:
> http://svn.apache.org/viewvc/james/protocols/trunk/smtp/src/main/java/org/apache/james/protocols/smtp/SMTPSession.java?view=markup
>
> See the pushLineHandler and popLineHandler methods to get an idea what
> I'm talking about.
>
> This would also make it easier to use the code with NIO or Blocking IO.
>
>> As I said in a previous mail there's an issue with read timeouts. IIUC the
>> client will be disconnected if it doesn't send anything for a certain amount
>> of time. When in IDLE I think we should disregard the usual timeout and use
>> another one or make the read timeout into a read/write timeout so that the
>> timer is reset both on reads and on writes and then send back an untagged
>> response every 30 seconds or so so the client doesn't timeout. This is what
>> the Dovecot IMAP server does. Every 30 seconds it sends back something like
>> '* OK Still here' to the client to prevent timeouts.
>
> The timeout should be handled by the NIO framework here. But I agree
> we need some special handling...
>
>>
>> Let me know what you think.
>>
>> /Niklas
>>
>> On 12/15/2010 05:46 PM, Norman Maurer wrote:
>>>
>>> Just a minor comment from a quick review.
>>>
>>> The synchronized(session) should be removed.. You use an AtomicBoolean
>>> so you should be safe here..
>>>
>>> Will have a deeper look later
>>>
>>> Bye,
>>> Nomrna
>>
>
> Bye,
> Norman
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
