james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Norman Maurer <nor...@apache.org>
Subject Re: svn commit: r1049622 - in /james/imap/trunk: api/src/main/java/org/apache/james/imap/api/ api/src/main/java/org/apache/james/imap/api/display/ message/src/main/java/org/apache/james/imap/decode/ message/src/main/java/org/apache/james/imap/decode/
Date Wed, 15 Dec 2010 17:46:57 GMT
Hi Niklas,

comments inside :)


2010/12/15 Niklas Therning <niklas@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-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message