Use UnsafeMutablePointer for handling z_stream

[Cyberbeni]

deflateEnd/inflateEnd returned Z_STREAM_ERROR, when passing &self.stream into them, they returned Z_OK but AFAIK it is not guaranteed in Swift that C structs won't get moved( https://developer.apple.com/videos/play/wwdc2016/720/?time=1085 ), using UnsafeMutablePointer solves this.

fixes hummingbird-project/hummingbird-compression#33

[adam-fowler]

Have you verified this makes a difference?

[Cyberbeni]

Yes, I added it to my Package.swift and did the same test as before (serve a basic site with a couple <100kB js/css files and force reload the page a lot of times).

[Cyberbeni]

On macOS, I modified this example for testing: https://github.com/sliemeobn/elementary-htmx/tree/main/Examples/HummingbirdDemo

Still some minor leaks but not related to Zlib.

[Cyberbeni]

[Cyberbeni]

(The project needs to be built with swift cli, building Package.swift from Xcode doesn't work with Instruments)

[adam-fowler]

Thanks for this. All looks good. I verified it works as well. One very minor thing

[adam-fowler]

Can we add a comment detailing why we need to use an UnsafeMutablePointer

[Cyberbeni]

Ok, added comment to both places.

[adam-fowler]

Thanks. This is great. It might be worthwhile pointing this out in the swift-nio-extras repository. They have some request/response compression channel handlers use Zlib in a similar manner.

[Cyberbeni]

I was already on the fence but then Apple decided that they want to play both sides (donating to Trump's inauguration, returning to advertising on X versus https://www.swift.org/diversity/ ), so I decided to not do any free QA for them unless absolutely necessary.

[adam-fowler]

Fair enough

[adam-fowler]

@Cyberbeni Was thinking last night we could have just got rid of the copy of the z_stream in the deinit and that would have been enough to fix the memory leak.

[Cyberbeni]

Yes but it could just as easily break again with just that change.

For example turning the wrapper into a struct would mean that copying/moving that changes the memory address, so changes outside of this file could break/fix the behavior.

I don't know if there is any magic behavior in the Swift runtime that would move stuff around in the memory but using UnsafeMutablePointer also prevents that.

C wrapper locks have been historically implemented the same way: https://github.com/apple/swift-metrics/blob/main/Sources/CoreMetrics/Locks.swift

[adam-fowler]

Its stored in a reference type so it is never copied

[Cyberbeni]

https://github.com/swiftlang/swift/blob/af3e7e765549c0397288e60983c96d81639287ed/stdlib/public/core/UnsafePointer.swift#L197-L205

According to this, you shouldn't access implicitly created unsafe pointers (such as passing &stream to the C function) after the function call finished. So even though just removing the copy in deinit seemingly fixes the issue, there is no guarantee that future Swift versions won't work differently or that the current Swift version doesn't have any hidden edge cases when that memory address changes at runtime.

[Cyberbeni]

For example just like how Hasher uses a random seed every time your app starts to prevent people from saving the hash (because if the hashing algorithm changes in a future version, that would change the hash of the same data anyways), Swift could generate a new virtual memory address every time there is an implicit unsafe pointer created and fatalError when that pointer is accessed after it's not supposed to be accessed.