diff --git a/concore.hpp b/concore.hpp index 339211c..ba676c0 100644 --- a/concore.hpp +++ b/concore.hpp @@ -142,7 +142,7 @@ class Concore{ /** * @brief Destructor for Concore class. - * Detaches and removes the shared memory segment if shared memory created. + * Detaches shared memory and removes it when the last process exits. */ ~Concore() { @@ -152,21 +152,8 @@ class Concore{ zmq_ports.clear(); #endif #ifdef __linux__ - // Detach the shared memory segment from the process - if (communication_oport == 1 && sharedData_create != nullptr) { - shmdt(sharedData_create); - } - if (communication_iport == 1 && sharedData_get != nullptr) { - shmdt(sharedData_get); - } - - // Remove the shared memory segment - if (shmId_create != -1) { - shmctl(shmId_create, IPC_RMID, nullptr); - } - if (semId_create != -1) { - semctl(semId_create, 0, IPC_RMID); - } + cleanupSharedMemory(shmId_create, sharedData_create, semId_create); + cleanupSharedMemory(shmId_get, sharedData_get, semId_get); #endif } @@ -213,14 +200,8 @@ class Concore{ return *this; #ifdef __linux__ - if (communication_oport == 1 && sharedData_create != nullptr) - shmdt(sharedData_create); - if (communication_iport == 1 && sharedData_get != nullptr) - shmdt(sharedData_get); - if (shmId_create != -1) - shmctl(shmId_create, IPC_RMID, nullptr); - if (semId_create != -1) - semctl(semId_create, 0, IPC_RMID); + cleanupSharedMemory(shmId_create, sharedData_create, semId_create); + cleanupSharedMemory(shmId_get, sharedData_get, semId_get); #endif s = std::move(other.s); @@ -304,6 +285,27 @@ class Concore{ __atomic_store(reinterpret_cast(base), &v, __ATOMIC_RELEASE); } + static void cleanupSharedMemory(int shm_id, char* shared_data, int sem_id) { + if (shared_data == nullptr) + return; + + if (!shm_sem_acquire(sem_id)) { + shmdt(shared_data); + return; + } + + shmdt(shared_data); + + struct shmid_ds shm_info; + if (shmctl(shm_id, IPC_STAT, &shm_info) == 0 && shm_info.shm_nattch == 0) { + shmctl(shm_id, IPC_RMID, nullptr); + semctl(sem_id, 0, IPC_RMID); + return; + } + + shm_sem_release(sem_id); + } + // Seqlock-style snapshot read: returns the payload on success, or // std::string() (empty) if the seq# is missing, odd (write in progress), // or changed between the two reads. The caller can retry without @@ -336,18 +338,19 @@ class Concore{ return id < 0 ? -1 : id; } - static void shm_sem_acquire(int id) { - if (id < 0) return; + static bool shm_sem_acquire(int id) { + if (id < 0) return false; sembuf sb{}; sb.sem_num = 0; sb.sem_op = -1; - sb.sem_flg = 0; + sb.sem_flg = SEM_UNDO; while (semop(id, &sb, 1) == -1) { if (errno != EINTR) { std::cerr << "semop(acquire) failed errno=" << errno << std::endl; - return; + return false; } } + return true; } static void shm_sem_release(int id) { @@ -355,7 +358,7 @@ class Concore{ sembuf sb{}; sb.sem_num = 0; sb.sem_op = 1; - sb.sem_flg = 0; + sb.sem_flg = SEM_UNDO; while (semop(id, &sb, 1) == -1) { if (errno != EINTR) { std::cerr << "semop(release) failed errno=" << errno << std::endl; @@ -372,9 +375,26 @@ class Concore{ */ void createSharedMemory(key_t key) { - shmId_create = shmget(key, SHM_SIZE, IPC_CREAT | 0666); + while (true) { + semId_create = shm_sem_create(key + 1); + if (semId_create < 0) { + std::cerr << "Failed to create shared memory semaphore." << std::endl; + return; + } + if (shm_sem_acquire(semId_create)) + break; + } + + bool created = false; + shmId_create = shmget(key, SHM_SIZE, IPC_CREAT | IPC_EXCL | 0666); + if (shmId_create != -1) { + created = true; + } else if (errno == EEXIST) { + shmId_create = shmget(key, SHM_SIZE, 0666); + } if (shmId_create == -1) { + shm_sem_release(semId_create); std::cerr << "Failed to create shared memory segment." << std::endl; return; } @@ -387,27 +407,27 @@ class Concore{ shmctl(shmId_create, IPC_RMID, nullptr); shmId_create = shmget(key, SHM_SIZE, IPC_CREAT | 0666); if (shmId_create == -1) { + shm_sem_release(semId_create); std::cerr << "Failed to recreate shared memory segment." << std::endl; return; } + created = true; } // Attach the shared memory segment to the process's address space sharedData_create = static_cast(shmat(shmId_create, NULL, 0)); if (sharedData_create == reinterpret_cast(-1)) { + shm_sem_release(semId_create); std::cerr << "Failed to attach shared memory segment." << std::endl; sharedData_create = nullptr; return; } - semId_create = shm_sem_create(key + 1); - if (semId_create < 0) { - std::cerr << "Failed to create shared memory semaphore." << std::endl; + if (created) { + shm_store_seq(sharedData_create, uint64_t{0}); + sharedData_create[SHM_HEADER_SIZE] = '\0'; } - - //initialise header - shm_store_seq(sharedData_create, uint64_t{0}); - sharedData_create[SHM_HEADER_SIZE] = '\0'; + shm_sem_release(semId_create); } /** @@ -420,37 +440,36 @@ class Concore{ int retry = 0; const int MAX_RETRY = 100; while (retry < MAX_RETRY) { - // Get the shared memory segment created by Writer + semId_get = semget(key + 1, 1, 0666); + if (semId_get == -1) { + std::cout << "Shared memory does not exist. Make sure the writer process is running." << std::endl; + sleep(1); + retry++; + continue; + } + + if (!shm_sem_acquire(semId_get)) { + retry++; + continue; + } + shmId_get = shmget(key, SHM_SIZE, 0666); - // Check if shared memory exists if (shmId_get != -1) { - break; // Break the loop if shared memory exists + sharedData_get = static_cast(shmat(shmId_get, NULL, 0)); + if (sharedData_get != reinterpret_cast(-1)) { + shm_sem_release(semId_get); + return; + } + sharedData_get = nullptr; } + shm_sem_release(semId_get); std::cout << "Shared memory does not exist. Make sure the writer process is running." << std::endl; - sleep(1); // Sleep for 1 second before checking again + sleep(1); retry++; } - if (shmId_get == -1) { - std::cerr << "Failed to get shared memory segment after max retries." << std::endl; - return; - } - - // Attach the shared memory segment to the process's address space - sharedData_get = static_cast(shmat(shmId_get, NULL, 0)); - if (sharedData_get == reinterpret_cast(-1)) { - std::cerr << "Failed to attach shared memory segment." << std::endl; - sharedData_get = nullptr; - return; - } - - //attach reader-side semaphore (writer owns its lifetime) - semId_get = semget(key + 1, 1, 0666); - if (semId_get < 0) { - //no semaphore: reads fall back to seq# alone - semId_get = -1; - } + std::cerr << "Failed to get shared memory segment after max retries." << std::endl; } #endif @@ -838,7 +857,8 @@ class Concore{ if (sharedData_create == nullptr) throw 506; #ifdef __linux__ - shm_sem_acquire(semId_create); + if (!shm_sem_acquire(semId_create)) + throw 507; #endif { auto* seqp = reinterpret_cast(sharedData_create); @@ -898,7 +918,8 @@ class Concore{ val.resize(max_payload); } #ifdef __linux__ - shm_sem_acquire(semId_create); + if (!shm_sem_acquire(semId_create)) + throw 507; #endif { auto* seqp = reinterpret_cast(sharedData_create); diff --git a/concoredocker.hpp b/concoredocker.hpp index 71fd843..108ba5f 100644 --- a/concoredocker.hpp +++ b/concoredocker.hpp @@ -130,14 +130,8 @@ class Concore { zmq_ports.clear(); #endif #ifdef __linux__ - if (communication_oport == 1 && sharedData_create != nullptr) - shmdt(sharedData_create); - if (communication_iport == 1 && sharedData_get != nullptr) - shmdt(sharedData_get); - if (shmId_create != -1) - shmctl(shmId_create, IPC_RMID, nullptr); - if (semId_create != -1) - semctl(semId_create, 0, IPC_RMID); + cleanupSharedMemory(shmId_create, sharedData_create, semId_create); + cleanupSharedMemory(shmId_get, sharedData_get, semId_get); #endif } @@ -184,14 +178,8 @@ class Concore { zmq_ports = std::move(other.zmq_ports); #endif #ifdef __linux__ - if (communication_oport == 1 && sharedData_create != nullptr) - shmdt(sharedData_create); - if (communication_iport == 1 && sharedData_get != nullptr) - shmdt(sharedData_get); - if (shmId_create != -1) - shmctl(shmId_create, IPC_RMID, nullptr); - if (semId_create != -1) - semctl(semId_create, 0, IPC_RMID); + cleanupSharedMemory(shmId_create, sharedData_create, semId_create); + cleanupSharedMemory(shmId_get, sharedData_get, semId_get); #endif iport = std::move(other.iport); @@ -275,6 +263,27 @@ class Concore { return v; } + static void cleanupSharedMemory(int shm_id, char* shared_data, int sem_id) { + if (shared_data == nullptr) + return; + + if (!shm_sem_acquire(sem_id)) { + shmdt(shared_data); + return; + } + + shmdt(shared_data); + + struct shmid_ds shm_info; + if (shmctl(shm_id, IPC_STAT, &shm_info) == 0 && shm_info.shm_nattch == 0) { + shmctl(shm_id, IPC_RMID, nullptr); + semctl(sem_id, 0, IPC_RMID); + return; + } + + shm_sem_release(sem_id); + } + static int shm_sem_create(key_t key) { // Try to create as the original owner. If it already exists, // attach without resetting its value. @@ -290,15 +299,16 @@ class Concore { return id < 0 ? -1 : id; } - static void shm_sem_acquire(int id) { - if (id < 0) return; + static bool shm_sem_acquire(int id) { + if (id < 0) return false; sembuf sb{}; sb.sem_num = 0; sb.sem_op = -1; - sb.sem_flg = 0; + sb.sem_flg = SEM_UNDO; while (semop(id, &sb, 1) == -1) { - if (errno != EINTR) return; + if (errno != EINTR) return false; } + return true; } static void shm_sem_release(int id) { @@ -306,15 +316,32 @@ class Concore { sembuf sb{}; sb.sem_num = 0; sb.sem_op = 1; - sb.sem_flg = 0; + sb.sem_flg = SEM_UNDO; while (semop(id, &sb, 1) == -1) { if (errno != EINTR) return; } } void createSharedMemory(key_t key) { - shmId_create = shmget(key, SHM_SIZE, IPC_CREAT | 0666); + while (true) { + semId_create = shm_sem_create(key + 1); + if (semId_create < 0) { + std::cerr << "Failed to create shared memory semaphore.\n"; + return; + } + if (shm_sem_acquire(semId_create)) + break; + } + + bool created = false; + shmId_create = shmget(key, SHM_SIZE, IPC_CREAT | IPC_EXCL | 0666); + if (shmId_create != -1) { + created = true; + } else if (errno == EEXIST) { + shmId_create = shmget(key, SHM_SIZE, 0666); + } if (shmId_create == -1) { + shm_sem_release(semId_create); std::cerr << "Failed to create shared memory segment.\n"; return; } @@ -326,51 +353,63 @@ class Concore { shmctl(shmId_create, IPC_RMID, nullptr); shmId_create = shmget(key, SHM_SIZE, IPC_CREAT | 0666); if (shmId_create == -1) { + shm_sem_release(semId_create); std::cerr << "Failed to recreate shared memory segment.\n"; return; } + created = true; } sharedData_create = static_cast(shmat(shmId_create, NULL, 0)); if (sharedData_create == reinterpret_cast(-1)) { + shm_sem_release(semId_create); std::cerr << "Failed to attach shared memory segment.\n"; sharedData_create = nullptr; return; } - semId_create = shm_sem_create(key + 1); - if (semId_create < 0) { - std::cerr << "Failed to create shared memory semaphore.\n"; + if (created) { + uint64_t zero = 0; + __atomic_store(reinterpret_cast(sharedData_create), &zero, __ATOMIC_RELEASE); + sharedData_create[SHM_HEADER_SIZE] = '\0'; } - - uint64_t zero = 0; - __atomic_store(reinterpret_cast(sharedData_create), &zero, __ATOMIC_RELEASE); - sharedData_create[SHM_HEADER_SIZE] = '\0'; + shm_sem_release(semId_create); } void getSharedMemory(key_t key) { int retry = 0; const int MAX_RETRY = 100; while (retry < MAX_RETRY) { + semId_get = semget(key + 1, 1, 0666); + if (semId_get == -1) { + std::cout << "Shared memory does not exist. Make sure the writer process is running.\n"; + sleep(1); + retry++; + continue; + } + + if (!shm_sem_acquire(semId_get)) { + retry++; + continue; + } + shmId_get = shmget(key, SHM_SIZE, 0666); - if (shmId_get != -1) - break; + if (shmId_get != -1) { + sharedData_get = static_cast(shmat(shmId_get, NULL, 0)); + if (sharedData_get != reinterpret_cast(-1)) { + shm_sem_release(semId_get); + return; + } + sharedData_get = nullptr; + } + + shm_sem_release(semId_get); std::cout << "Shared memory does not exist. Make sure the writer process is running.\n"; sleep(1); retry++; } - if (shmId_get == -1) { - std::cerr << "Failed to get shared memory segment after max retries.\n"; - return; - } - sharedData_get = static_cast(shmat(shmId_get, NULL, 0)); - if (sharedData_get == reinterpret_cast(-1)) { - std::cerr << "Failed to attach shared memory segment.\n"; - sharedData_get = nullptr; - return; - } - semId_get = semget(key + 1, 1, 0666); - if (semId_get < 0) semId_get = -1; + + std::cerr << "Failed to get shared memory segment after max retries.\n"; } #endif @@ -557,7 +596,8 @@ class Concore { << "-byte shared memory limit. Data truncated!" << std::endl; result.resize(SHM_PAYLOAD_MAX); } - shm_sem_acquire(semId_create); + if (!shm_sem_acquire(semId_create)) + throw 507; { auto* seqp = reinterpret_cast(sharedData_create); (void)__atomic_fetch_add(seqp, uint64_t{1}, __ATOMIC_ACQ_REL); // odd = writing diff --git a/tests/test_shm_lifecycle.py b/tests/test_shm_lifecycle.py new file mode 100644 index 0000000..76a7806 --- /dev/null +++ b/tests/test_shm_lifecycle.py @@ -0,0 +1,162 @@ +import os +import shutil +import subprocess +import sys +import tempfile +import textwrap +from pathlib import Path + +import pytest + + +REPO_ROOT = Path(__file__).resolve().parent.parent + +pytestmark = pytest.mark.skipif( + shutil.which("g++") is None, + reason="g++ not available", +) + + +@pytest.fixture(autouse=True) +def _skip_windows(): + if sys.platform == "win32": + pytest.skip("SHM requires POSIX") + + +@pytest.mark.parametrize( + ("header", "key"), + [("concore.hpp", 543210), ("concoredocker.hpp", 543212)], +) +def test_shared_segment_lives_until_last_writer_exits(header, key): + with tempfile.TemporaryDirectory(prefix="concore_shm_test_") as temp_dir: + temp_path = Path(temp_dir) + key += os.getpid() * 2 + (temp_path / "concore.oport").write_text(f'{{"{key}": "1"}}', encoding="utf-8") + + source_file = temp_path / "shm_lifecycle_test.cpp" + binary_file = temp_path / "shm_lifecycle_test" + source_file.write_text( + textwrap.dedent( + f""" + #include "{header}" + #include + #include + #include + #include + #include + #include + #include + + int probe(key_t key) {{ + int shm_id = shmget(key, 4096, 0666); + int sem_id = semget(key + 1, 1, 0666); + if (shm_id == -1 || sem_id == -1) + return 2; + + char* data = static_cast(shmat(shm_id, nullptr, 0)); + if (data == reinterpret_cast(-1)) + return 3; + std::string payload(data + 8, strnlen(data + 8, 4087)); + shmdt(data); + return payload == "[0,42]" ? 0 : 4; + }} + + int main(int argc, char** argv) {{ + std::string mode = argv[1]; + key_t key = static_cast(std::stoi(argv[2])); + if (mode == "probe") + return probe(key); + if (mode == "cleanup") {{ + int shm_id = shmget(key, 4096, 0666); + int sem_id = semget(key + 1, 1, 0666); + if (shm_id != -1) + shmctl(shm_id, IPC_RMID, nullptr); + if (sem_id != -1) + semctl(sem_id, 0, IPC_RMID); + return 0; + }} + + Concore concore; + if (mode == "hold") {{ + concore.delay = 0; + concore.simtime = 0; + concore.write(1, "payload", std::vector{{42}}); + }} + if (mode == "hold" || mode == "wait") {{ + std::cout << "ready" << std::endl; + std::cin.get(); + }} + return 0; + }} + """ + ).lstrip(), + encoding="utf-8", + ) + + compile_result = subprocess.run( + [ + "g++", + "-std=c++17", + "-I", + str(REPO_ROOT), + "-o", + str(binary_file), + str(source_file), + ], + capture_output=True, + text=True, + timeout=60, + cwd=temp_path, + ) + if compile_result.returncode != 0: + pytest.fail(f"g++ compile failed:\n{compile_result.stderr}") + + command = [str(binary_file)] + subprocess.run(command + ["cleanup", str(key)], cwd=temp_path, check=True) + writers = [] + + def start_writer(mode): + writer = subprocess.Popen( + command + [mode, str(key)], + cwd=temp_path, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + text=True, + ) + writers.append(writer) + assert writer.stdout.readline().strip() == "ready" + return writer + + try: + creator = start_writer("hold") + attached = start_writer("wait") + assert ( + subprocess.run(command + ["probe", str(key)], cwd=temp_path).returncode + == 0 + ) + attached.communicate(input="\n", timeout=5) + assert attached.returncode == 0 + assert ( + subprocess.run(command + ["probe", str(key)], cwd=temp_path).returncode + == 0 + ) + + attached = start_writer("wait") + creator.communicate(input="\n", timeout=5) + assert creator.returncode == 0 + assert ( + subprocess.run(command + ["probe", str(key)], cwd=temp_path).returncode + == 0 + ) + attached.communicate(input="\n", timeout=5) + assert attached.returncode == 0 + assert ( + subprocess.run(command + ["probe", str(key)], cwd=temp_path).returncode + == 2 + ) + finally: + for writer in writers: + if writer.poll() is None: + writer.kill() + writer.wait() + subprocess.run(command + ["cleanup", str(key)], cwd=temp_path, check=False)