Calling sem_init on named semaphores is not the intended use-case for sem_init

[drbitboy]

ImageStreamIO/ImageStreamIO.c

Line 1712 in d6f9c28

sem_init(

sem_init is suppose to be applied to unnamed semaphors; see here and here.

[drbitboy]

Hi guys,

Sanford told me someone said I am mis-classifying the unnamed semaphores in ImageStreamIO.c as named semphores.

Since the memory for all semaphores in ImageStreamIO.c is allocated by sem_open, they are named semaphores; subsequently using sem_init on them does not change or remove the name.

Also note that, if they were not named semaphores, then processes other than the producer that creates them would not be able to find, open (via sem_open) and use them.

[oguyon]

Yes, indeed, our semaphores are named. This is the correct choice: as you point out we can access them across processes. The milk-streamCTRL tool relies on this as well.

[DasVinch]

@drbitboy no problem with cleaning out the code with proper use of sem_init and sem_open. Semaphores for shared images should be named as @oguyon said.

This brings up a vague concern with a restriction with non-shared images (we're carrying a shared=0 option throughout ImageSteamIO).
Currently, non-shared images have no semaphores at all. The use of unnamed semaphores could nonetheless be useful there if Image were ever to be used in multi-thread, intra-process applications.
Thoughts on that?

[oguyon]

Would be easy to create unnamed semaphores if shared=0. Looking into possible future upgrades, it would be nice to be able to deploy the same code as either multi-process (using named semaphores) and multi-threaded (using unnamed semaphores). Having shared=0 streams create unnamed semaphores is a small necessary step in this direction... so I am reopening this issue.

[jaredmales]

Isn't the more pressing question: why does the current code do what it does? It's calling sem_init only after a successful call to sem_open with O_CREAT, thus re-initializing an already initialized semaphore. From the docs:

Initializing a semaphore that has already been initialized results in undefined behavior.

This is done after every call to sem_open--O_CREAT. I think the sem_init can and should just be removed.

[drbitboy]

I was thinking of a different solution ...

Background

ImageStreamIO currently manages, implicitly or explicitly, several shared memory files:

1. the shmim itself in directory [getenv MILK_SHARED_DIR], OR macro SHAREDMEMDIR ≡ "/milk/shm" OR "/tmp/",
   1.1. comprising
   1.1.1. the data themselves (circular buffer of "images"),
   1.1.2. plus the IMAGE_METADATA struct members,
   1.1.3. plus more: keywords; PIDs; and others, perhaps some perhaps not even used.
   1.2. With per-process pointers in the IMAGE struct e.g.
   1.2.1. e.g. [IMAGE].md (IMAGE_METADATA*) pointer to the metadata struct in the shmim
   1.2.2. e.g. [IMAGE].array.raw/.UI8/.SI8/.UI16/... union of pointers to the shmim image data/circular buffer
   1.2.3. e.g. [IMAGE].kw (IMAGE_KEYWORD*) pointer to keywords in the shmim
   1.2.4. etc.

2. the semaphore data themselves, comprising several shared memory (plural) sem.{name}_semNN (
   2.1. where NN runs from 0 to [IMAGE].md->sem-1
   2.2. and with a per-process malloc'ed array of NN (sem_t*) pointers (i.e. pointers to pointers) of the NN semaphore data, named [IMAGE].semptr

Proposed refactor

Shift the semaphore data from NBsem separate semaphore files to the shmim with three lines of code:

sharedsize += image->md->sem * sizeof(sem_t);
...
image->semptr = map;
map +=  image->md->sem * sizeof(sem_t);

Pros

• Completely eliminates all of the semaphore name and file management in item (2) above
  • no more mallocs and initializing an array of pointers.
  • No more monkey business with O_CREAT and O_EXCL (btw, regarding which I am not convinced was completely resolve in the recent PR)
• semaphore creation, management, and destruction is implicit with shmim creation, management, and destruction
  • N.B. this means my upcoming refactor using INODE to change shmims cleanly on the fly, e.g. for a shmim size change, will not have to fiddle around separately with semaphore INODEs, because the semaphores are essentially encapsulated within, and get updated with, the shmim.
• no more stale semaphore files hanging around when a process gets an unhandled signal.
• only the semaphore creator, which is folded into the shmim creator, needs to call sem_init on each semaphore datum.

Cons

• There are no longer (file-)named semaphores
  • but
    • A) that's a good thing because of the Pros above, and
    • B) that does not matter because any processes interested can find these semaphores via the (file-)named shmim anyway.
• All ImageStreamIO library clients must use the ImageStreamIO_sem*(...) calls to access and use the semaphores
  • but they should be doing that anyway.
  • IIRC, shmimMonitor in MagAOX is a violator here: it uses the system call sem_timedwait; this is easily fixed.
• I am playing a little fast and loose with the semptr assignment code above, which now becomes a pointer to the first of an array of (sem_t) structs, instead of a pointer to the first of an array of (sem_t*) pointers.
  • So again, any ImageStreamIO library clients that are accessing shmim semaphore internals directly, instead of via the ImageStreamIO_sem*(...) calls, may break.