Don't allocate when adding attachments. #616
Conversation
|
Failing test Message:
Contains failed.
Expected: True
But was: False
Stack Trace:
MultipartRelatedTests.TestReferenceByContentId() line 151Indecently Message:
Initial Root
Expected: <X-MimeKit-Warning: Do NOT use ToString() to serialize entities! Use one of the WriteTo() methods instead!
Content-Type: text/html; charset=utf-8
Content-Id: <7F64657G3CU4.1KUI3FL529CE3@Mjölnir>
This is the html body...>
But was: <X-MimeKit-Warning: Do NOT use ToString() to serialize entities! Use one of the WriteTo() methods instead!
Content-Type: image/gif
Content-Disposition: inline; filename=empty.gif
Content-Id: <Z3H2657G3CU4.QIGWC03L02CU3@Mjölnir>
>
Stack Trace:
MultipartRelatedTests.TestDocumentRoot() line 70 |
MimeKit/AttachmentCollection.cs
Outdated
| filter.Filter (buf, 0, nread, out index, out length); | ||
| content.Write (buf, 0, nread); | ||
| } | ||
|
|
||
| filter.Flush (buf, 0, 0, out index, out length); | ||
|
|
||
| ArrayPool<byte>.Shared.Return (buf); |
There was a problem hiding this comment.
I'd usually recommend wrapping the above code (while loop and flush) inside a try block and then putting the return inside a finally to ensure that it's returned despite potential exceptions. It'll get collected eventually but it's the pattern I've seen recommended.
There was a problem hiding this comment.
Yeah it's all managed memory so the GC will collect the buffer eventually, that's why I didn't bother since the exception would go unhandled and try.. finally.. gives a false sense of security.
I could add the pattern though if that's what everyone wants.
There was a problem hiding this comment.
I must admit, I've not explored the usage of this and whether exceptions may be caught higher up. I'd assumed that might be the case and if so, there's a slight benefit to putting it back in the pool cleanly.
There was a problem hiding this comment.
I'll add the pattern then (I haven't explored either tbh.)
Co-authored-by: Anton Firszov <[email protected]>
|
I've fixed that unit test. Does |
|
FWIW, I originally did not use System.Buffers because it was not available on all of the platforms that MimeKit originally supported. I think that all of those .NET platforms have since been dropped, but I will need to verify. If System.Buffers is available on all supported platforms, then I will be able to drop https://github.com/jstedfast/MimeKit/blob/master/MimeKit/Utils/BufferPool.cs (effectively the same idea). |
@jstedfast Yes, the length can be greater so when working with buffers directly so you should ensure that you do not rely on that property. Ideally you'd wrap the pool and work directly with span slices only falling back to the array when you need it. |
minor stylistic changes
|
I'll work on updating more areas of the code to use ArrayPool. |
|
Might be an idea to define some sort of implementation similar to |
|
Do you have an example of IMemoryOwner being used? Is the idea just to replace the need for the try/finally syntax and replacing it with a using block? |
|
BTW, just spotted this: Is that your machine's hostname? Looks like I might need to encode the hostname if so. I sure hope it's that and not some sort of memory corruption. |
|
Yeah Mjölnir 😄 |
|
Cool, cool :) I just fixed GenerateMessageId() to encode the hostname if it is international. |
Hi, I don't know if you're looking for this kind of PR but I spotted the allocation when F12ing through the methods in Visual Studio.
A quick search for
new byte[indicates that there are quite a few places in the library where the same pattern could be used e.g.FilteredStream.Read. Unfortunately I cannot currently dedicate the time to replace each instance so I thought I would introduce an example fix for this instance.