chibenwa commented on pull request #886: URL: https://github.com/apache/james-project/pull/886#issuecomment-1059707355
https://stackoverflow.com/questions/9082101/do-messages-sent-via-nettys-channel-write-preserve-their-order-when-begin-sen Answer picked up from [this issue](https://github.com/netty/netty/issues/3887). (Thanks Scottmitch@github). Netty can provide "ordering" in the following situations: - You are doing all writes from the EventLoop thread; OR - You are doing no writes from the EventLoop thread (i.e. all writes are being done in other thread(s)). It is subtle but important thing to remember as it can cause painful bugs. If some channel.write() calls are coming from application threads and some are coming from EventLoop then the writes can seem out of order from application's perspective. See more details on the linked issue. In the context of list, untagged response are delegated to reactor, which in the case of the distributed James server is scheduled on an Elastic thread. In `ListProcessor`: ``` getMailboxManager().search( MailboxQuery.builder() .userAndNamespaceFrom(basePath) .expression(new PrefixedRegex( basePath.getName(), ModifiedUtf7.decodeModifiedUTF7(mailboxName), mailboxSession.getPathDelimiter())) .build(), Minimal, mailboxSession) // processResult sends untagged notification from an elastic thread .doOnNext(metaData -> processResult(responder, isRelative, metaData, getMailboxType(session, metaData.getPath()))) .then() .block(); // OK is currently sent from the event loop. okComplete(request, responder); ``` Note that processResult sends untagged responses. So the conclusions are: - We don't need to await writes nor chain futures - The fix is similar to the performance problems mentionned above. Solution is to switch `ImapChannelUpstreamHandler` out of the event loop. Something like (`ImapServer`): ``` public class IMAPServer extends AbstractConfigurableAsyncServer implements ImapConstants, IMAPServerMBean, NettyConstants { private EventExecutorGroup eventExecutors; // ... @Override public void doConfigure(HierarchicalConfiguration<ImmutableNode> configuration) throws ConfigurationException { // ... eventExecutors = new DefaultEventExecutorGroup(maxExecutorThreads, NamedThreadFactory.withName("imapserver")); } // ... @Override protected AbstractChannelPipelineFactory createPipelineFactory() { return new AbstractChannelPipelineFactory(getFrameHandlerFactory()) { // ... @Override public void initChannel(Channel channel) { //... pipeline.addLast(eventExecutors, CORE_HANDLER, createHandler()); } }; } // ... } ``` I will propose those changes on a separate pull request, that @Arsnael will be able to try, and, once validated, that you would be able to cherry-pick. Good weekend, Cheers -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
