gkm2164 commented on PR #3144: URL: https://github.com/apache/james-project/pull/3144#issuecomment-5474531768
Closing this one: validating it against the full `protocols-imap4` suite showed the approach here is wrong, and the reworked fix is different enough that a fresh PR will be easier to review than a rewrite of this branch. For the record, what this patch got wrong: it bounded each read to the literal size reported by `NotEnoughDataException#getNeededSize()`. That value is `size + read + crlf`, where `read` only counts characters pulled through `nextChar()` — bytes already consumed by an earlier `read(int, boolean)` on the same line are not included. So for a command carrying several literals it under-reports, and anything that trusts it precisely breaks (`IMAPServerSearchTest#passing2literalOnSameNetworkPackageWhenMoreThan16Chars` deadlocks). The existing "read everything, compare loosely" code is not sloppy — it is what makes that inaccuracy harmless, which I did not appreciate when I opened this. The rework keeps that failure path exactly as it is today and instead fixes only the success path, which is where the actual bug lives: parse against a non-consuming view of Netty's cumulation, then advance the reader index by what the command actually used, so a pipelined follow-up command is left in the buffer instead of being dropped by `pending.clear()`. That needs no size arithmetic at all and removes the `should_buffer` bookkeeping. I will open a new PR once it is green locally. Apologies for the CI cycles spent on this branch. -- 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]
