Fix response parsing when response starts with a newline

maxoly commented 7 months ago

Fix IMAP response parsing when responses are received in fragments.

Motivation:

The application I'm developing uses NWConnection to communicate with an IMAP server. Occasionally, the server (or NWConnection?) may split the IMAP response into fragments, even when maximumLength is set to a high value in the nwConnection.receive method, making such fragmentation unnecessary. In these cases, subsequent parts of the response might begin with a new line, causing parsing issues. For example:

First part:

* 142 FETCH (UID 12001 INTERNALDATE "03-Feb-2024 10:37:09 +0000")
* 143 FETCH (UID 12022 INTERNALDATE "07-Feb-2024 06:25:44 +0000")

Second and final part:


* 144 FETCH (UID 12023 INTERNALDATE "07-Feb-2024 10:27:28 +0000")
* 145 FETCH (UID 12024 INTERNALDATE "07-Feb-2024 12:42:19 +0000")
* 146 FETCH (UID 12025 INTERNALDATE "07-Feb-2024 14:17:31 +0000")
TAG4 OK Success

While the first part is parsed correctly, the second part results in a parsing error. To address this issue, I have implemented optional parsing for a newline at the beginning of the response.

Modifications:

Enhanced the IMAP response parser to optionally ignore a leading newline character when processing fragmented responses. This change ensures that each fragment is correctly parsed as part of the continuous response, regardless of how NWConnection or the server decides to fragment the data.
Added unit tests that simulate receiving fragmented responses, including cases where fragments start with a newline character.

Result:

After this change, the application can correctly parse IMAP responses received in fragments, even when a fragment starts with a newline. This improvement prevents parsing errors in fragmented response scenarios, ensuring reliable communication with the IMAP server and enhancing the application's resilience to varying network behaviours.

danieleggert commented 5 months ago

Thanks for the PR — and sorry for the super late response.

This situation should have been handled by the FramingParser. There could be a bug in the FramingParser, though.

Are you passing your data into that?

maxoly commented 5 months ago

Hi @danieleggert, thank you for getting back to me!

I'm not currently using the FramingParser for handling the data. Instead, I'm utilizing NWConnection.receive(minimumIncompleteLength: maximumLength:) to manage incoming data streams. After receiving the data, I pass it directly to an instance of NIOSingleStepByteToMessageProcessor<NIOResponseDecoder> for processing.

danieleggert commented 5 months ago

Hi @danieleggert, thank you for getting back to me!

I'm not currently using the FramingParser for handling the data. Instead, I'm utilizing NWConnection.receive(minimumIncompleteLength: maximumLength:) to manage incoming data streams. After receiving the data, I pass it directly to an instance of NIOSingleStepByteToMessageProcessor<NIOResponseDecoder> for processing.

You need to somehow use the FramingParser before calling into the ResponseParser.

I’m not sure what the right fix is, but ResponseParser is intentionally not handling this, because it’s only supposed to operate on the output of FramingParser. This should be called out in docs. I can look into that.

How are you using NIOSingleStepByteToMessageProcessor / NIOResponseDecoder? I’m using the core part of this parser instead of the “outer wrapper”, but those “outer wrappers” are supposed to work and also use the FramingParser + ResponseParser combination.

danieleggert commented 2 months ago

Let me know if you want help with this. We should probably have better documentation on how to do this.

Closing this for now.

apple / swift-nio-imap