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

MacOS: fix usage of libdispatch and Availability macros in Thread #1523

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

barracuda156
Copy link

  1. Fix usage of Availability macros: they should not be prefixed with underscores (those are used by internal headers, unprefixed work correctly always).
  2. Do not assume libdispatch is always available, that is not the case. Use it where it actually exists.

@barracuda156 barracuda156 changed the title Darwin dispatch fix MacOS: fix usage of libdispatch and Availability macros in Thread Sep 17, 2024
@icraggs
Copy link
Contributor

icraggs commented Sep 18, 2024

As far as I can see there is no alternative implementation if HAS_DISPATCH returns false. What happens then?

@barracuda156
Copy link
Author

As far as I can see there is no alternative implementation if HAS_DISPATCH returns false. What happens then?

@icraggs If it is not defined, macro which checks for OSX and HAS_DISPATCH evaluates to false, and a generic posix fallback is used. Conditions under which it gets defined are specified in the header.
Do I miss something here?

I ran the build with and without enabling dispatch, both cases worked. (I have a custom libdispatch which is hidden from environment unless explicitly asked for.)

@icraggs
Copy link
Contributor

icraggs commented Sep 20, 2024

The question is does the posix fallback work on MacOS? I remember now - MacOS doesn't support unnamed semaphores, so the fallback won't work. I think we'd need another section for MacOS without the dispatch calls.

@barracuda156
Copy link
Author

@icraggs If you could help with switching that to Mach semaphores, that would be great.

I recall mentions about some issues with unnamed semaphores on macOS, though I probably did not encounter actual breakages due to them.

@fpagliughi
Copy link
Contributor

fpagliughi commented Jan 13, 2025

As an alternate overall implementation, consider that semaphores seem to be used as just binary semaphores to be a signal for simple events. That is actually the same usage as how the condition variables are implemented in the library - not to loop and wait on a condition, but to simply wait on the signal, as a simple event flag.

Thus the current implementation of semaphores and condition variables in the library are fairly redundant and can be consolidated.

They could both be replaced with with a single object type that I would suggest would more properly be called an "Event" object. This could be a fairly uniform implementation across platforms, since Windows also has condition variables. But perhaps the Windows implementation could just use the OS Event auto-reset object, since it matches the usage perfectly.

The *nix (non-Windows) implementation can be similar to the existing condition variable code, but I would suggest adding a boolean field as the "signaled" condition and add proper looping checks to avoid problems with spurious wakeups.

I already have a working implementation in a branch in my private fork, here:

https://github.com/fpagliughi/paho.mqtt.c/blob/windows-cond-var/src/Thread.c

My original intent was to explore the use of Windows condition variables to wrap the use of lists into encapsulated, thread-protected, signaled objects, like a ThreadList that internally manages a lock and signal for when items are added and removed.

But this is more exploratory, The code to consolidate semaphores and condition variables into an "Event" object is a nice cleanup, and appears to be working fine. It's really more of a cleanup of existing code than anything novel.

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.

3 participants