Skip to content

Conversation

@Hibyehello
Copy link

There were problems on MacOS when trying to create an NSWindow because I wasn't doing it on the main thread so to solve this I added a check if it's the main thread and it not use dispatch sync instead.

@btzy
Copy link
Owner

btzy commented Oct 4, 2025

I think this issue might have been raised before, and at that time I didn't implement it because I felt that it was the responsibility of the caller to use the main thread. Accepting this PR would imply that NFD is thread-aware, which means that we will need to document if and how multiple calls to NFD APIs can be made simultaneously (from separate threads). As of now the implementation has non-local static variables so it might be undefined behaviour to call some pairs of functions simultaneously.

If this is guaranteed to work as expected in all reasonable macOS applications then maybe we can consider it. Could this cause a deadlock in a situation where it would previously just return NFD_ERROR?

Copy link
Owner

@btzy btzy left a comment

Choose a reason for hiding this comment

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

Can we just modify the NFD_{}N_With_Impl family of functions? There should be five of them; I think all the other functions call those five eventually.

@Hibyehello
Copy link
Author

Oh sorry, didn't see the notification. I'm not sure how often multiple of the functions would need to be called concurrently, the use case I ran into, prompting this pr, was the I had a separate ui thread from the main thread which is what is calling NFD and you physically can't create a window not on the main thread, not sure if other OSes have this same limitation.

A deadlock could theoretically occur if the main thread was blocked by the window needing to be opened but somehow not opened already, but from my testing I haven't had any problems.

Can we just modify the NFD_{}N_With_Impl family of functions? There should be five of them; I think all the other functions call those five eventually.

I wasn't looking too close and honestly probably rushed this pr way to quickly, but yeah we could bring it to the NFD_{}_WithN_Impl,

I can clean that up if you would like.

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.

2 participants