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

Detach from any threads we have attached to. #173

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Conversation

lhoward
Copy link
Contributor

@lhoward lhoward commented Nov 9, 2024

Fixes: #172

@lhoward lhoward force-pushed the lhoward/tsd branch 2 times, most recently from e0ab587 to c922cbe Compare November 9, 2024 17:08
@ktoso
Copy link
Collaborator

ktoso commented Nov 11, 2024

Oh boy, this will be quite some trouble outside of simple examples (where we'll have to deal with the fact that Task{} and friends exist)... because if we can't make JNI calls from a thread that did not register, technically after every suspension point we may be on a different thread, so we may need to keep trying to register threads.

We also have no control over dispatch deciding to start/stop threads, so we may leave lingering resources since we'd never detach some of the threads hm.

I think in the long term we're going to need to write an executor, make it the global and main executor and there we'd have to control threads and this way we'd be able to detach them consistently... 🤔 We are considering offering APIs to customize the global default executors so this may be possible nicely in the future, but that's not until next Swift release hm.

Meanwhile I'll think about your idea here.

@lhoward
Copy link
Contributor Author

lhoward commented Nov 11, 2024

This relates to #157 – if we remove the affinity between projected objects and environments, then we can always attach a thread on demand (and register a detach destructor). I'm not sure what the right thing to do is when there are multiple VMs, but the JNI spec is quite clear that environments are pre-thread (FWIW, Android only ever has a single VM).

@lhoward
Copy link
Contributor Author

lhoward commented Nov 11, 2024

Given the necessary hooks in the Swift runtime (I haven't looked too closely), we could definitely extend our AExecutor to be a concurrent executor. But whilst idiomatic, a platform-agnostic approach that uses a thread pool which attaches/detaches should be fine.

The elephant in the room here though is going to be third-party libraries that use libdispatch instead of Swift concurrency. (I've already had to carefully audit the dependencies in my application to ensure that nothing calls DispatchQueue.main.async() as the main queue is never started on Android.) As long as it is not too expensive, I think a solution that attaches on demand and supports the continued use of libdispatch is desirable.

@ktoso
Copy link
Collaborator

ktoso commented Nov 12, 2024

Heh main.async() is a pretty funny wrinkle. Having that said, if we had proper Swift concurrency adopting libraries they'd do MainActor and then we're in the middle of allowing the executor for that actor -- so we'd be able to then replace it with some more meaningful thread on android, and not have to ban using it. This won't be in Swift 6.0 though, but is something we're investigating

@lhoward lhoward marked this pull request as ready for review November 17, 2024 06:57
@lhoward
Copy link
Contributor Author

lhoward commented Nov 18, 2024

CI error appears unrelated

<unknown>:0: error: unknown argument: '-abi-comments-in-module-interface'
<unknown>:0: error: unknown argument: '-abi-comments-in-module-interface'

@ktoso
Copy link
Collaborator

ktoso commented Nov 18, 2024

Gotta update the branch, can you do that please :)

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

I think this is okey for the time being, if we find better ways of doing this we can adjust of course.

@ktoso
Copy link
Collaborator

ktoso commented Nov 19, 2024

Thanks for the contrib!

@ktoso ktoso merged commit 28fb69f into swiftlang:main Nov 19, 2024
12 checks passed
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.

JavaKit: thread detach interactions with implicitly created threads
2 participants