Fix macOS and Windows shared mem wait for detach #7321
Conversation
TheMarex
left a comment
TheMarex
left a comment
There was a problem hiding this comment.
Looks good, some smaller questions. Thanks for breaking it out!
include/storage/shared_memory.hpp
Outdated
| throw util::exception("shmctl encountered an error: " + errorToMessage(error_code) + | ||
| SOURCE_REF); | ||
| ::shmid_ds xsi_ds; | ||
| if (::shmctl(xsi.get_shmid(), IPC_STAT, &xsi_ds) < 0) |
There was a problem hiding this comment.
If I read this correctly, in the case of an error we swallow that and just return? Previously it would have thrown an exception.
There was a problem hiding this comment.
But we are expecting an error here. The error being that the region is gone.
There was a problem hiding this comment.
But can we check for a precise error code in that case? There may be other reasons this call errors.
There was a problem hiding this comment.
If we got there, the only plausible error that still can occur is: "EIDRM shmid points to a removed identifier". And that is exactly what we are waiting for to happen. I can test for this error and throw if any other error happens.
There was a problem hiding this comment.
IMHO it makes it a bit more clear that we are waiting for something specific to happen.
include/storage/shared_memory.hpp
Outdated
| #else | ||
| // Windows - specific code | ||
|
|
||
| // POSIX shared mem |
There was a problem hiding this comment.
I don't remember exactly why we used XSI over POSIX. Maybe issues with OSX at some point? To be honest we were originally planning to drop the whole hot-swapping thing in favor of mmap. The hotswap/shmem was conceived almost 15 years ago when the typical deployment process was "run it on a big server", not rolling deployments behind a load balancer.
Would have been nice for the 6.0 to clean this up but alas.
MarcelloPerathoner
force-pushed
the
fix-shared-memory-macwin
branch
from
d738492 to
8bc6e77
Compare
2 participants
Issue
Fixes #7320
On macOS and Windows
osrm-datastorenow correctly waits for other processes to detach from shared memory before exiting. Previous behaviour:osrm-datastoreslept for 50ms and then boldly announced: 'All clients switched.'This caused
Tasklist
Requirements / Relations
Related to PR #7309