Skip to content

Conversation

JRR-OSU
Copy link
Contributor

@JRR-OSU JRR-OSU commented Jul 12, 2022

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!

@lexuanquynh
Copy link

thanks you. I hope that will merge soon.

@samrayner
Copy link

Thank you so much @JRR-OSU!

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

@morzv
Copy link

morzv commented Sep 22, 2022

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

@horpot
Copy link

horpot commented Sep 29, 2022

Hello, does anyone know when this will be merged?

@tkausch
Copy link

tkausch commented Oct 6, 2022

@Goos @samrayner Hello

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

@murraygoodwin
Copy link

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

@beckychristensen
Copy link
Contributor

beckychristensen commented Oct 11, 2022

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 beckychristensen merged commit d354880 into envoy:master Oct 11, 2022
@horpot
Copy link

horpot commented Oct 12, 2022

Thank you! :)

@samrayner
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants