Skip to content

Fix ownership of CFSocketCreateRunLoopSource - #6089

Merged
rolfbjarne merged 17 commits into
mainfrom
Therzok-patch-2
Nov 15, 2022
Merged

Fix ownership of CFSocketCreateRunLoopSource#6089
rolfbjarne merged 17 commits into
mainfrom
Therzok-patch-2

Conversation

@Therzok

@Therzok Therzok commented May 21, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@monojenkins

This comment was marked as outdated.

Sign in to view
rolfbjarne
rolfbjarne requested changes May 21, 2019
View reviewed changes

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test would be nice.

See example here: https://github.com/xamarin/xamarin-macios/pull/6091/files

@Therzok
Therzok force-pushed the Therzok-patch-2 branch from bdd83b6 to a8468ab Compare May 23, 2019 21:02
@Therzok

Therzok commented May 23, 2019

Copy link
Copy Markdown
Contributor Author

I just noticed something. CFSocketInvalidate is not called, so the runloopsource is never removed. So the native leak of CFSocket will still be there. Should we bind CFSocket Invalidate to allow users to invalidate the socket before disposal?

@Therzok

Therzok commented May 23, 2019

Copy link
Copy Markdown
Contributor Author

There's a few other cans of worms I won't open in this PR:
a) The info pointer should actually contain a release call to the GCHandle, not Dispose
b) The info pointer is actually the wrong type.

@monojenkins

This comment was marked as outdated.

Sign in to view
@monojenkins

This comment was marked as outdated.

Sign in to view
@Therzok
Therzok force-pushed the Therzok-patch-2 branch from 435ec32 to 6e7c1d8 Compare May 24, 2019 12:58
rolfbjarne
rolfbjarne requested changes May 24, 2019
View reviewed changes
Comment thread src/CoreFoundation/CFSocket.cs Outdated
Comment thread src/CoreFoundation/CFSocket.cs Outdated
Comment thread src/CoreFoundation/CFSocket.cs Outdated
Comment thread tests/monotouch-test/CoreFoundation/SocketTest.cs
@monojenkins

This comment was marked as outdated.

Sign in to view
@rolfbjarne
rolfbjarne changed the base branch from master to main June 12, 2020 09:03
@monojenkins

This comment was marked as outdated.

Sign in to view
@rolfbjarne
Merge remote-tracking branch 'origin/main' into HEAD
20a02f3
@rolfbjarne
[CFSocket] A few changes/improvements/updates.
8643244 
* Use function pointers for the new callbacks in .NET.
* Misc nullability fixes.
* Simplify the construction logic a bit, and make it not leak a GCHandle in case
  something goes wrong.
* Change the Invalidate method to call Dispose, and call CFSocketInvalidate from
  Dispose, since CFSocketInvalidate must always be called at the end according to
  Apple's documentation.
* Adjust the new test accordingly.
* Add a new test verifying that CFSockets are collected by the GC.
* This new test revealed a mistake in reference counting (the context's reference
  count should start out at 0, not 1), which has been fixed.
* Rename a few methods and fields to be more explicit.
* Update code according to general changes since this PR was introduced.
@Therzok

Therzok commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for taking over ❤️

@rolfbjarne
rolfbjarne removed the request for review from spouliot October 31, 2022 14:54
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Nov 10, 2022

Copy link
Copy Markdown
Collaborator

❗ API diff for current PR / commit (Breaking changes)

Legacy Xamarin (No breaking changes)
.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)

❗ API diff vs stable (Breaking changes)

Legacy Xamarin (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 16f34746c328774d2fc1b4bf100f62a62125aa8c [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Nov 10, 2022

Copy link
Copy Markdown
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: 16f34746c328774d2fc1b4bf100f62a62125aa8c [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Nov 10, 2022

Copy link
Copy Markdown
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1103.Monterey
Hash: 16f34746c328774d2fc1b4bf100f62a62125aa8c [PR build]

@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Nov 11, 2022

Copy link
Copy Markdown
Collaborator

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

0 tests crashed, 1 tests failed, 222 tests passed.

Failures

❌ monotouch tests [attempt 2]

1 tests failed, 22 tests passed.
Details
  • monotouch-test/iOS Unified 64-bits - simulator/Debug: TimedOut

Html Report (VSDrops) Download

Successes

✅ bcl: All 69 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

dalexsoto
dalexsoto approved these changes Nov 11, 2022
View reviewed changes

@dalexsoto dalexsoto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

mandel-macaque
mandel-macaque reviewed Nov 11, 2022
View reviewed changes
Comment thread src/CoreFoundation/CFSocket.cs
} else if (cbType == CFSocketCallBackType.NoCallBack) {
// nothing to do
} else if (cbType == CFSocketCallBackType.ReadCallBack) {
} else if (cbType == CFSocketCallBackType.ReadCallBack && socket.ReadEvent != null) {

@mandel-macaque mandel-macaque Nov 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (cbType == CFSocketCallBackType.ReadCallBack && socket.ReadEvent != null) {
} else if (cbType == CFSocketCallBackType.ReadCallBack && socket.ReadEvent is not null) {

mandel-macaque
mandel-macaque reviewed Nov 11, 2022
View reviewed changes
Comment thread src/CoreFoundation/CFSocket.cs
} else if (cbType == CFSocketCallBackType.ReadCallBack && socket.ReadEvent != null) {
socket.OnRead (new CFSocketReadEventArgs ());
} else if (cbType == CFSocketCallBackType.WriteCallBack) {
} else if (cbType == CFSocketCallBackType.WriteCallBack && socket.WriteEvent != null) {

@mandel-macaque mandel-macaque Nov 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (cbType == CFSocketCallBackType.WriteCallBack && socket.WriteEvent != null) {
} else if (cbType == CFSocketCallBackType.WriteCallBack && socket.WriteEvent is not null) {

@rolfbjarne
rolfbjarne merged commit 4025f4e into main Nov 15, 2022
@rolfbjarne
rolfbjarne deleted the Therzok-patch-2 branch November 15, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dalexsoto dalexsoto dalexsoto approved these changes
@rolfbjarne rolfbjarne rolfbjarne approved these changes
+1 more reviewer
@mandel-macaque mandel-macaque mandel-macaque approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@rolfbjarne rolfbjarne

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Therzok @monojenkins @vs-mobiletools-engineering-service2 @dalexsoto @rolfbjarne @mandel-macaque