Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when releasing dispatch sources in GSMultiHandle #37

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

will-velazquez
Copy link

@will-velazquez will-velazquez commented Dec 12, 2024

The logic here comes from readdle/swift-corelibs-foundation@819eab9#diff-ab36fc9e1b50ee4f5efa7d24cb3f806160e52c31f97724605527faab21a52bbbR465

More details: swiftlang/swift-corelibs-foundation#4791 (comment)

We use a separate queue for read/write sources which allows us to cancel & release them synchronously

This fixes the first problem mentioned but note that

In case of socket connection timeout, _MultiHandle will get invalid (closed) socket handle from cURL, and Dispatch would crash trying to work with that handle. We have no workaround for this other than suppressing crashes in Dispatch, unfortunately. Also, cURL version was bumped since then, so this have to be re-checked.

Is still an issue. There's a fix for that available that avoids using dispatch altogether and implements a different Sources event emitter for Windows specifically, but in practice I haven't seen this be an issue and the code is complicated. We are also bandaging over by commenting out the assertion in libDispatch

FYI the 'real' fix is in 0c1a3e8
The followup cleanup commit is just to simplify setting up and tearing down the socket sources since tracing through the original code was unnecessarily complicated

The logic here comes from readdle/swift-corelibs-foundation@819eab9#diff-ab36fc9e1b50ee4f5efa7d24cb3f806160e52c31f97724605527faab21a52bbbR465

We use a separate queue for read/write sources which allows us to cancel & release them synchronously
1. Moves private impl details to .m
2. Provide 1:1 curl callback methods
3. Simplify read/write source logic and get rid of unnecessary GSSocketRegisterAction
Rather than 5% leeway, we were giving 0.000005% leeway
Per the docs, URLCache should retain, not copy
@will-velazquez will-velazquez merged commit 5e74ac5 into master Dec 13, 2024
3 of 10 checks passed
@will-velazquez will-velazquez deleted the will/nsurlsession-crash branch December 24, 2024 21:00
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.

1 participant