Embassy icon indicating copy to clipboard operation
Embassy copied to clipboard

Fix crashes related to breaking Swift language changes to withMemoryRebound in Xcode 14

Open JRR-OSU opened this issue 3 years ago • 1 comments

This PR fixes crashes presumably related to S3-0333 which adjusted the behavior of withMemoryRebound. These crashes prevent us from using this framework in Xcode 14 beta likely due to SE-0333 being accepted and included in its release.

Explanation

According to the docs of withMemoryRebound the parameter count refers to "the number of instances of Pointee to bind to type." This means that it does not actually refer to the size of the rebound region, but instead to the number of instances of T being mapped within it. The previous implementation passed the size of the MemoryLayout rather than simply passing 1, which is the number of socketaddr instances we're attempting to map/bind to.

The proposal linked previously asserted that these changes would not break source compatibility, but I have found this assumption to be flawed. Even though TCPSocket's previous implementation appeared to misunderstand the intent of the capacity parameter, it functioned nonetheless. I am willing to file an issue to the Swift team if there's anyone that can help me distill this issue down.

My fix has seemed to resolved the crashes on our end, but I would gladly appreciate others to verify my understanding. I am not too well-accustomed to C land, so it is entirely possible my understanding is flawed as well.

In any case, if these changes can be verified and merged as soon as possible, we would be much appreciative, as we would love to continue using this library in Xcode 14 onward.

Thank you!

JRR-OSU avatar Jul 12 '22 20:07 JRR-OSU

thanks you. I hope that will merge soon.

lexuanquynh avatar Jul 27 '22 01:07 lexuanquynh

Thank you so much @JRR-OSU!

@Goos Please can you consider merging this so we can continue to use the library with Xcode 14?

samrayner avatar Sep 14 '22 14:09 samrayner

@Goos Hi, can you merge this PR, please? it is blocker for usage the library with Xcode 14.

morzv avatar Sep 22 '22 11:09 morzv

Hello, does anyone know when this will be merged?

horpot avatar Sep 29 '22 08:09 horpot

@Goos @samrayner Hello

Any plans to merge this PR. I like this project and do not want to fork it... Thanks a lot!

tkausch avatar Oct 06 '22 16:10 tkausch

Costa Coffee loves this project too! We’ve applied this change but it would be great to have the PR merged in 🙏🏻

murraygoodwin avatar Oct 06 '22 19:10 murraygoodwin

Sorry for the delay on this, and thanks @JRR-OSU for providing a fix! I'll create a new release including this fix tomorrow

beckychristensen avatar Oct 11 '22 19:10 beckychristensen

Thank you! :)

horpot avatar Oct 12 '22 06:10 horpot

Awesome, thank you so much @beckychristensen! Look forward to the release.

samrayner avatar Oct 12 '22 10:10 samrayner