MultipartKit V5

[ptoffy]

No description provided.

[adam-fowler]

Some initial comments.

[ptoffy]

@adam-fowler Do you want to take a look at this again and see if stuff makes more sense now? I added in some binary data (which even contains hex-CRLF 😄) to the tests so it should be able to parse anything now

[ptoffy]

@Joannis @simonjbeaumont @czechboy0 pulling you into this so you can take a look if you want

[Joannis]

This makes a copy from string, can't we use withContiguousMemoryIfAvailable?

[ptoffy]

Mhh I don't think that would work any better because we need the Collection there. We can't pass in the raw bytes and if we're copying them to an array we're still making a copy at that point right?

[Joannis]

Since you're doing an .append on the parser you're right that you're already making a copy. Except right now you're making two copies of the same data, and each time you're also allocating space for that data.

[ptoffy]

No, I mean we can't pass the raw pointer to the decode method because it expects the collection of bytes, so this can't be done

try string.utf8.withContiguousStorageIfAvailable { bytes in
    decode(D.self, from: bytes, boundary: boundary)
} ?? decode(D.self, from: Array(string.utf8), boundary: boundary)

And if we were to do something like

if let bytes = string.utf8.withContiguousStorageIfAvailable(Array.init) {
    try decode(D.self, from: bytes, boundary: boundary)
} else {
    try decode(D.self, from: Array(string.utf8), boundary: boundary)
}

we're still initialising an array with the raw bytes so I think this doesn't really save us a copy.
Unless you mean a different way of using withContiguousStorageIfAvailable

[Joannis]

I would really prefer not copying unnecessarily in a library like this

[Joannis]

Is it necessary to copy data out before parsing?

[ptoffy]

This way we avoid a whole bunch of needMoreData returns from the parser

[Joannis]

Could it be restructured to be parseOrAppend then?

if parserBuffer.isEmpty {
  let result = parser.parse(data)
  if result == .needMoreData {
    parser.append(data)
  }
} else {
  parser.append(data)
  parser.parse()
}

[ptoffy]

But why? We're just doing that once at the beginning, this is the sync parse

[adam-fowler]

Generic parameter changes look good

[adam-fowler]

As I understand it the user can receive a body as multiple MultipartSections if the underlying AsyncSequence has broken that body up. This is great as we don't want to pay the memory for large bodies if we can. But there are situations where we want to ensure we have a complete body section eg a block of data we want to run a JSON decode on. Is it possible to add a helper function to the Iterator to do this? eg Iterator.collectBody(upTo: memoryLimit) which returns a header, complete body section

[ptoffy]

There's currently a parse method on MultipartParser which loads the input all at once. I'm guessing you mean something of a middle ground between this and stream parsing? E.g just for one part of the message

[adam-fowler]

Yes just one part of the message eg I have a Multipart message with a zip file in there plus some metadata. I want to save the zip files to disk (using the least amount of memory possible ie streaming it) but parse the metadata with Codable so need the whole of it in memory.

[ptoffy]

Added 38bc3c1#diff-0d53ff899089448c57c2915f2315bbf9d9e50f9a16caa3e2669249e84ff87d35R76

[0xTim]

Some comments. This is starting to look good and we're getting close to be able to tag an alpha I think.

Would be good to get the performance tests set up so we can measure changes going forward and code coverage to see which edge cases we don't currently test

[adam-fowler]

Looking good. Maybe we should have two async sequences though. One that does the collation automatically and one that requires you to use nextCollatedPart when you want a collated part.

I would rename the current AsyncSequence StreamingMultipartParserAsyncSequence and add the following

public struct MultipartParserAsyncSequence<BackingSequence: AsyncSequence>: AsyncSequence
where BackingSequence.Element: MultipartPartBodyElement & RangeReplaceableCollection {
    let streamingSequence: StreamingMultipartParserAsyncSequence<BackingSequence>

    public init(boundary: String, buffer: BackingSequence) {
        self.streamingSequence = .init(boundary: boundary, buffer: buffer)
    }

    public struct AsyncIterator: AsyncIteratorProtocol {
        public mutating func next() async throws -> MultipartSection<BackingSequence.Element>? {
            try await self.streamingIterator.nextCollatedPart()
        }

        var streamingIterator: StreamingMultipartParserAsyncSequence<BackingSequence>.AsyncIterator
    }

    public func makeAsyncIterator() -> AsyncIterator {
        return .init(streamingIterator: self.streamingSequence.makeAsyncIterator())
    }
}

Most people won't care about streaming sections of the multipart file, so the default should provide the collated parts. But if someone wants to stream the body part of a file they can use the StreamingMultipartParserAsyncSequence version.

[adam-fowler]

Why are you re-initializing the header fields, its a variable local to this function

[ptoffy]

Well if there's different parts in a message there might other headers later and this empties them before possibly reading other ones

[adam-fowler]

Is there no chance you can read a header part after having read body parts?

[ptoffy]

Yes there is if the message has different parts

[ptoffy]

FYI in this case fields will always have just one field because the original sequence returns single header fields and body chunks. I'm not sure if this is misleading and should be clarified somehow, either with docs or with a name change

[ptoffy]

I agree with the proposed changes but I moved the nextCollatedPart() to simply be the next() method of the new sequence which makes more sense to me

[0xTim]

Nothing blocking on my end. Once other comments have been resolved we can look at integrating this into projects to see how it actually works

[Joannis]

Instead of copying the boundary into an array, write the leading tokens, boundary and CRLF separately. Saves copies and allocs, thus a lot of work.

[Joannis]

Same here, you can easily prevent a copy

[Joannis]

Unnecessary copy

[Joannis]

CRLF is copied into this a lot. I would keep it as "\r\n".utf8 and append those values. Don't copy it into an array if possible

[Joannis]

My main (only) issue is that this library makes a lot of copies out of every byte carrier (String, Array etc)