krakjoe / apcu

APCu - APC User Cache
Other
957 stars 194 forks source link

POSIX-style shm_open/mmap with ".shm" filemask not implemented #381

Open diablodale opened 5 years ago

diablodale commented 5 years ago

Hi. While reviewing the code, I think I noticed that the apc.mmap_file_mask feature...

POSIX-style shm_open/mmap put a ".shm" somewhere in your mask. eg. "/apc.shm.XXXXXX"

is not implemented.

https://github.com/krakjoe/apcu/blob/32aaf38b2474f2f4cfea763d626e59c96086b957/INSTALL#L263

I can find no call shm_open() in the codebase. I also can't find any substring searches for "shm" on the file_mask in the codebase. 🤔

And related...since the mmap_file_mask code creates a random temporary filename...

https://github.com/krakjoe/apcu/blob/998157ac57ffe768a4023f22f32886aba5bcbcbd/apc_mmap.c#L87

that means another process (e.g. apache worker processes) will never know and use that same exact filename. Instead, another process will get another random filename and map a unique new shared file. So each process has their own unique memory-mapped shareable sections that are shared with no other process.

To get a shared-across-process effect, I think what is needed is to not create random temporary files. Instead, something similar to http://pubs.opengroup.org/onlinepubs/009695399/functions/shm_open.html or https://nullprogram.com/blog/2016/04/10/#posix-memory-mapping

if (strstr(mmap_file_mask, ".shm")) {
  int fd = shm_open(mmap_file_mask, ...);
  pSharedMem = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, fd, 0);
}

Naturally, the mutex/lock code must already be in place and working for this shared-across-process to be successful. ;-)

Did I overlook something? Comments?

nikic commented 5 years ago

The relevant code has been removed in https://github.com/krakjoe/apcu/commit/998157ac57ffe768a4023f22f32886aba5bcbcbd (also based on random files).

That could be brought back if there's motivation -- what is it that you want to do here? apcu currently works on the assumption of either thread-based or fork-based model (in the latter case the sharing is cross-process).

diablodale commented 5 years ago

Ah, there is was. Got it, the INSTALL doc is outdated. If I wrote a PR to remove the old text referring to this ".shm", would you consider accepting it?

My motivation was coherency between code and doc, I saw something claimed but not implemented. It meant one or the other wasn't in sync. :-)

nikic commented 5 years ago

If I wrote a PR to remove the old text referring to this ".shm", would you consider accepting it?

Sure! There's probably more outdated stuff in there -- for example the introduction refers to support for PHP 4 and PHP 5, even though only PHP 7 is supported nowadays ^^