From b228089fc6468a33014786e7d72fe6e5595085c0 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Wed, 17 Jul 2024 21:06:48 -0500 Subject: [PATCH] Split common Shm logic to base class And decouple the close and unlink functions. There was a lot of duplicate code between ShmLinux, ShmMacOS, and ShmWindows, which is now in the internal ShmBase class. This made it easier to have consistent logic around the close vs. unlink behavior across platforms, with less copy/pasted code. --- src/main/java/org/apposed/appose/NDArray.java | 2 +- .../java/org/apposed/appose/SharedMemory.java | 22 ++- .../java/org/apposed/appose/ShmFactory.java | 13 +- .../java/org/apposed/appose/shm/ShmBase.java | 130 ++++++++++++++ .../java/org/apposed/appose/shm/ShmLinux.java | 110 +++--------- .../java/org/apposed/appose/shm/ShmMacOS.java | 117 ++++-------- .../org/apposed/appose/shm/ShmWindows.java | 166 ++++++------------ 7 files changed, 274 insertions(+), 286 deletions(-) create mode 100644 src/main/java/org/apposed/appose/shm/ShmBase.java diff --git a/src/main/java/org/apposed/appose/NDArray.java b/src/main/java/org/apposed/appose/NDArray.java index 6257de7..62e19b4 100644 --- a/src/main/java/org/apposed/appose/NDArray.java +++ b/src/main/java/org/apposed/appose/NDArray.java @@ -33,7 +33,7 @@ import java.util.Arrays; /** - * Represents a multi-dimensional array similar to NumPy ndarray. + * Represents a multidimensional array similar to a NumPy ndarray. *

* The array contains elements of a {@link DType data type}, arranged in a * particular {@link Shape}, and flattened into {@link SharedMemory}, diff --git a/src/main/java/org/apposed/appose/SharedMemory.java b/src/main/java/org/apposed/appose/SharedMemory.java index 3bbba9b..073d4d5 100644 --- a/src/main/java/org/apposed/appose/SharedMemory.java +++ b/src/main/java/org/apposed/appose/SharedMemory.java @@ -106,7 +106,7 @@ static SharedMemory createOrAttach(String name, boolean create, int size) { * * @return The length in bytes of the shared memory. */ - long size(); + int size(); /** * JNA pointer to the shared memory segment. @@ -115,19 +115,29 @@ static SharedMemory createOrAttach(String name, boolean create, int size) { */ Pointer pointer(); + /** + * Sets whether the {@link #unlink()} method should be invoked to destroy + * the shared memory block when the {@link #close()} method is called. + *

+ * By default, shared memory objects constructed with {@link #create} will + * behave this way, whereas shared memory objects constructed with + * {@link #attach} will not. But this method allows to override the behavior. + *

+ */ + void unlinkOnClose(boolean unlinkOnClose); + /** * Requests that the underlying shared memory block be destroyed. * In order to ensure proper cleanup of resources, unlink should be * called once (and only once) across all processes which have access * to the shared memory block. */ - default void unlink() { - throw new UnsupportedOperationException(); - } + void unlink(); /** - * Closes access to the shared memory from this instance but does - * not destroy the shared memory block. + * Closes access to the shared memory from this instance, + * but does not necessarily destroy the shared memory block. + * See also {@link #unlinkOnClose(boolean)}. */ @Override void close(); diff --git a/src/main/java/org/apposed/appose/ShmFactory.java b/src/main/java/org/apposed/appose/ShmFactory.java index 6fabda1..717d436 100644 --- a/src/main/java/org/apposed/appose/ShmFactory.java +++ b/src/main/java/org/apposed/appose/ShmFactory.java @@ -28,6 +28,17 @@ */ package org.apposed.appose; +/** + * Interface for platform-specific creation of {@link SharedMemory} instances. + *

+ * Each platform (e.g. Linux, macOS, Windows) provides its own implementation + * of this interface, which knows how to manufacture {@link SharedMemory} blocks + * for that platform. These implementations are declared as implementations in + * {@code META-INF/services/org.apposed.appose.ShmFactory}, so that Java's + * {@code ServiceLoader} can discover them in an extensible way, and then use + * the one best suited for the platform at hand. + *

+ */ public interface ShmFactory { -SharedMemory create(String name, boolean create, int size); + SharedMemory create(String name, boolean create, int size); } diff --git a/src/main/java/org/apposed/appose/shm/ShmBase.java b/src/main/java/org/apposed/appose/shm/ShmBase.java new file mode 100644 index 0000000..83b6349 --- /dev/null +++ b/src/main/java/org/apposed/appose/shm/ShmBase.java @@ -0,0 +1,130 @@ +/*- + * #%L + * Appose: multi-language interprocess cooperation with shared memory. + * %% + * Copyright (C) 2023 - 2024 Appose developers. + * %% + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * #L% + */ + +package org.apposed.appose.shm; + +import com.sun.jna.Pointer; +import org.apposed.appose.SharedMemory; + +/** + * Base class for platform-specific shared memory implementations. + * + * @author Carlos Garcia Lopez de Haro + * @author Tobias Pietzsch + * @author Curtis Rueden + */ +abstract class ShmBase implements SharedMemory { + + /** Struct containing shm details, including name, size, pointer(s), and handle. */ + protected final ShmInfo info; + + /** Whether the memory block has been closed. */ + private boolean closed; + + /** Whether the memory block has been unlinked. */ + private boolean unlinked; + + protected ShmBase(final ShmInfo info) { + this.info = info; + } + + protected abstract void doClose(); + protected abstract void doUnlink(); + + @Override + public String name() { + return info.name; + } + + @Override + public int size() { + return info.size; + } + + @Override + public Pointer pointer() { + return info.pointer; + } + + @Override + public void unlinkOnClose(boolean unlinkOnClose) { + info.unlinkOnClose = unlinkOnClose; + } + + @Override + public synchronized void unlink() { + if (unlinked) throw new IllegalStateException("Shared memory '" + info.name + "' is already unlinked."); + doClose(); + doUnlink(); + unlinked = true; + } + + @Override + public synchronized void close() { + if (closed) return; + doClose(); + if (info.unlinkOnClose) doUnlink(); + closed = true; + } + + @Override + public String toString() { + StringBuilder sb = new StringBuilder(); + sb.append(getClass().getSimpleName()).append("{"); + sb.append("name='").append(name()).append("', size=").append(size()); + if (info.pointer != null) sb.append(", pointer=").append(info.pointer); + if (info.writePointer != null) sb.append(", writePointer=").append(info.writePointer); + if (info.handle != null) sb.append("handle=").append(info.handle); + sb.append(", closed=").append(closed); + sb.append(", unlinked=").append(unlinked); + sb.append("}"); + return sb.toString(); + } + + /** Struct containing details about this shared memory block. */ + protected static class ShmInfo { + + /** Unique name that identifies the shared memory segment. */ + String name; + + /** Size in bytes. */ + int size; + + /** Pointer referencing the shared memory. */ + Pointer pointer; + + Pointer writePointer; + + /** File handle for the shared memory block's (pseudo-)file. */ + HANDLE handle; + + /** Whether to destroy the shared memory block when {@link #close()} is called. */ + boolean unlinkOnClose; + } +} diff --git a/src/main/java/org/apposed/appose/shm/ShmLinux.java b/src/main/java/org/apposed/appose/shm/ShmLinux.java index 893dd0d..81fba54 100644 --- a/src/main/java/org/apposed/appose/shm/ShmLinux.java +++ b/src/main/java/org/apposed/appose/shm/ShmLinux.java @@ -47,12 +47,10 @@ /** * Linux-specific shared memory implementation. - *

- * TODO separate unlink and close - *

* * @author Carlos Garcia Lopez de Haro * @author Tobias Pietzsch + * @author Curtis Rueden */ public class ShmLinux implements ShmFactory { @@ -62,35 +60,32 @@ public SharedMemory create(final String name, final boolean create, final int si return new SharedMemoryLinux(name, create, size); } - private static class SharedMemoryLinux implements SharedMemory { + private static class SharedMemoryLinux extends ShmBase { - /** - * File descriptor - */ - private final int fd; - - /** - * Size in bytes - */ - private final int size; + // name without leading slash + private SharedMemoryLinux(final String name, final boolean create, final int size) { + super(prepareShm(name, create, size)); + } - /** - * Pointer referencing the shared memory - */ - private final Pointer pointer; + @Override + protected void doUnlink() { + LibRtOrC.shm_unlink(name()); + } - /** - * Unique name that identifies the shared memory segment. - */ - private final String name; + @Override + protected void doClose() { + // Unmap the shared memory + if (pointer() != Pointer.NULL && LibRtOrC.munmap(pointer(), size()) == -1) { + throw new RuntimeException("munmap failed. Errno: " + Native.getLastError()); + } - /** - * Whether the memory block has been closed and unlinked - */ - private boolean unlinked = false; + // Close the file descriptor + if (LibRtOrC.close(info.handle) == -1) { + throw new RuntimeException("close failed. Errno: " + Native.getLastError()); + } + } - // name without leading slash - private SharedMemoryLinux(final String name, final boolean create, final int size) { + private static ShmInfo prepareShm(String name, boolean create, int size) { String shm_name; long prevSize; if (name == null) { @@ -123,49 +118,12 @@ private SharedMemoryLinux(final String name, final boolean create, final int siz throw new RuntimeException("mmap failed, errno: " + Native.getLastError()); } - this.size = shm_size; - this.name = withoutLeadingSlash(shm_name); - this.fd = shmFd; - this.pointer = pointer; - } - - @Override - public String name() { - return name; - } - - @Override - public Pointer pointer() { - return pointer; - } - - @Override - public long size() { - return size; - } - - /** - * Unmap and close the shared memory. Necessary to eliminate the shared memory block - */ - @Override - public synchronized void close() { - if (unlinked) { - return; - } - - // Unmap the shared memory - if (this.pointer != Pointer.NULL && LibRtOrC.munmap(this.pointer, size) == -1) { - throw new RuntimeException("munmap failed. Errno: " + Native.getLastError()); - } - - // Close the file descriptor - if (LibRtOrC.close(this.fd) == -1) { - throw new RuntimeException("close failed. Errno: " + Native.getLastError()); - } - - // Unlink the shared memory object - LibRtOrC.shm_unlink(this.name); - unlinked = true; + ShmInfo info = new ShmInfo<>(); + info.size = shm_size; + info.name = withoutLeadingSlash(shm_name); + info.handle = shmFd; + info.pointer = pointer; + return info; } /** @@ -202,18 +160,6 @@ private static long getSHMSize(final int shmFd) { return size; } - @Override - public String toString() { - return "ShmLinux{" + - "fd=" + fd + - ", size=" + size + - ", pointer=" + pointer + - ", name='" + name + '\'' + - ", unlinked=" + unlinked + - '}'; - } - - private static class LibRtOrC { /** diff --git a/src/main/java/org/apposed/appose/shm/ShmMacOS.java b/src/main/java/org/apposed/appose/shm/ShmMacOS.java index a3a9daa..7f4fc87 100644 --- a/src/main/java/org/apposed/appose/shm/ShmMacOS.java +++ b/src/main/java/org/apposed/appose/shm/ShmMacOS.java @@ -45,12 +45,10 @@ /** * MacOS-specific shared memory implementation. - *

- * TODO separate unlink and close - *

* * @author Carlos Garcia Lopez de Haro * @author Tobias Pietzsch + * @author Curtis Rueden */ public class ShmMacOS implements ShmFactory { @@ -60,8 +58,30 @@ public SharedMemory create(final String name, final boolean create, final int si return new SharedMemoryMacOS(name, create, size); } - private static class SharedMemoryMacOS implements SharedMemory { + private static class SharedMemoryMacOS extends ShmBase { private SharedMemoryMacOS(final String name, final boolean create, final int size) { + super(prepareShm(name, create, size)); + } + + @Override + protected void doUnlink() { + CLibrary.INSTANCE.shm_unlink(name()); + } + + @Override + protected void doClose() { + // Unmap the shared memory + if (pointer() != Pointer.NULL && CLibrary.INSTANCE.munmap(pointer(), size()) == -1) { + throw new RuntimeException("munmap failed. Errno: " + Native.getLastError()); + } + + // Close the file descriptor + if (CLibrary.INSTANCE.close(info.handle) == -1) { + throw new RuntimeException("close failed. Errno: " + Native.getLastError()); + } + } + + private static ShmInfo prepareShm(String name, boolean create, int size) { String shm_name; long prevSize; if (name == null) { @@ -75,7 +95,7 @@ private SharedMemoryMacOS(final String name, final boolean create, final int siz } ShmUtils.checkSize(shm_name, prevSize, size); - // shmFd = INSTANCE.shm_open(this.memoryName, O_RDWR, 0666); + // shmFd = INSTANCE.shm_open(this.memoryName, O_RDWR, 0666); final int shmFd = MacosHelpers.INSTANCE.create_shared_memory(shm_name, size); if (shmFd < 0) { throw new RuntimeException("shm_open failed, errno: " + Native.getLastError()); @@ -89,78 +109,14 @@ private SharedMemoryMacOS(final String name, final boolean create, final int siz throw new RuntimeException("mmap failed, errno: " + Native.getLastError()); } - this.size = shm_size; - this.name = withoutLeadingSlash(shm_name); - this.fd = shmFd; - this.pointer = pointer; + ShmInfo info = new ShmInfo<>(); + info.size = shm_size; + info.name = withoutLeadingSlash(shm_name); + info.handle = shmFd; + info.pointer = pointer; + return info; } - /** - * File descriptor - */ - private final int fd; - - /** - * Size in bytes - */ - private final int size; - - /** - * Pointer referencing the shared memory - */ - private final Pointer pointer; - - /** - * Unique name that identifies the shared memory segment. - */ - private final String name; - - /** - * Whether the memory block has been closed and unlinked - */ - private boolean unlinked = false; - - @Override - public String name() { - return name; - } - - @Override - public Pointer pointer() { - return pointer; - } - - @Override - public long size() { - return size; - } - - /** - * Unmap and close the shared memory. Necessary to eliminate the shared memory block - */ - @Override - public synchronized void close() { - if (unlinked) { - return; - } - - // Unmap the shared memory - if (this.pointer != Pointer.NULL && CLibrary.INSTANCE.munmap(this.pointer, size) == -1) { - throw new RuntimeException("munmap failed. Errno: " + Native.getLastError()); - } - - // Close the file descriptor - if (CLibrary.INSTANCE.close(this.fd) == -1) { - throw new RuntimeException("close failed. Errno: " + Native.getLastError()); - } - - // Unlink the shared memory object - CLibrary.INSTANCE.shm_unlink(this.name); - unlinked = true; - } - - // name without leading slash - /** * Try to open {@code name} and get its size in bytes. * @@ -194,16 +150,5 @@ private static long getSHMSize(final int shmFd) { } return size; } - - @Override - public String toString() { - return "ShmMacOS{" + - "fd=" + fd + - ", size=" + size + - ", pointer=" + pointer + - ", name='" + name + '\'' + - ", unlinked=" + unlinked + - '}'; - } } } diff --git a/src/main/java/org/apposed/appose/shm/ShmWindows.java b/src/main/java/org/apposed/appose/shm/ShmWindows.java index d64ebdd..d6b056e 100644 --- a/src/main/java/org/apposed/appose/shm/ShmWindows.java +++ b/src/main/java/org/apposed/appose/shm/ShmWindows.java @@ -52,145 +52,95 @@ public class ShmWindows implements ShmFactory { // name is WITHOUT prefix etc + @Override public SharedMemory create(final String name, final boolean create, final int size) { if (ShmUtils.os != ShmUtils.OS.WINDOWS) return null; // wrong platform return new SharedMemoryWindows(name, create, size); } - private static class SharedMemoryWindows implements SharedMemory { - String shm_name; - long prevSize; + private static class SharedMemoryWindows extends ShmBase { + private SharedMemoryWindows(final String name, final boolean create, final int size) { - if(name ==null) + super(prepareShm(name, create, size)); + } + + @Override + protected void doClose() { + cleanup(info.pointer, info.writePointer, info.handle); + } + + @Override + protected void doUnlink() { + // Note: The shared memory object will be deleted when all processes + // have closed their handles to it. There's no direct "unlink" + // equivalent in Windows like there is in POSIX systems. + } - { + private static ShmInfo prepareShm(String name, boolean create, int size) { + String shm_name; + long prevSize; + if (name == null) { do { shm_name = ShmUtils.make_filename(SHM_SAFE_NAME_LENGTH, SHM_NAME_PREFIX_WIN); prevSize = getSHMSize(shm_name); } while (prevSize >= 0); - } else - - { + } else { shm_name = nameMangle_TODO(name); prevSize = getSHMSize(shm_name); } ShmUtils.checkSize(shm_name, prevSize, size); - final WinNT.HANDLE shm_hMapFile = Kernel32.INSTANCE.CreateFileMapping( - WinBase.INVALID_HANDLE_VALUE, - null, - WinNT.PAGE_READWRITE, - 0, - size, - shm_name + final WinNT.HANDLE hMapFile = Kernel32.INSTANCE.CreateFileMapping( + WinBase.INVALID_HANDLE_VALUE, + null, + WinNT.PAGE_READWRITE, + 0, + size, + shm_name ); - if(hMapFile ==null) - - { + if (hMapFile == null) { throw new RuntimeException("Error creating shared memory array. CreateFileMapping failed: " + Kernel32.INSTANCE.GetLastError()); } - final int shm_size = (int) getSHMSize(shm_hMapFile); + final int shm_size = (int) getSHMSize(hMapFile); // Map the shared memory Pointer pointer = Kernel32.INSTANCE.MapViewOfFile( - hMapFile, - WinNT.FILE_MAP_WRITE, - 0, - 0, - size + hMapFile, + WinNT.FILE_MAP_WRITE, + 0, + 0, + size ); - if(pointer ==null) - - { + if (isNull(pointer)) { Kernel32.INSTANCE.CloseHandle(hMapFile); throw new RuntimeException("Error creating shared memory array. " + Kernel32.INSTANCE.GetLastError()); } Pointer writePointer = Kernel32.INSTANCE.VirtualAllocEx(Kernel32.INSTANCE.GetCurrentProcess(), - pointer, new BaseTSD.SIZE_T(size), WinNT.MEM_COMMIT, WinNT.PAGE_READWRITE); - if(writePointer ==null) - - { - close(); + pointer, new BaseTSD.SIZE_T(size), WinNT.MEM_COMMIT, WinNT.PAGE_READWRITE); + if (isNull(writePointer)) { + cleanup(pointer, writePointer, hMapFile); throw new RuntimeException("Error committing to the shared memory pages. Errno: " + Kernel32.INSTANCE.GetLastError()); } - this.size =shm_size; - this.name = - - nameUnmangle_TODO(shm_name); - this.hMapFile =shm_hMapFile; - this.pointer =pointer; - this.writePointer =writePointer; - } - - /** - * reference to the file that covers the shared memory region - */ - private WinNT.HANDLE hMapFile; - - /** - * Size in bytes - */ - private final int size; - - /** - * Pointer referencing the shared memory - */ - private final Pointer pointer; - - private final Pointer writePointer; - - /** - * Unique name that identifies the shared memory segment. - */ - private final String name; - - /** - * Whether the memory block has been closed and unlinked - */ - private boolean unlinked = false; - - @Override - public String name() { - return name; - } - - @Override - public Pointer pointer() { - return pointer; - } - - @Override - public long size() { - return size; - } - - /** - * Unmap and close the shared memory. Necessary to eliminate the shared memory block - */ - @Override - public synchronized void close() { - if (unlinked) { - return; - } - if (writePointer != null) { - Kernel32.INSTANCE.UnmapViewOfFile(this.writePointer); - } - Kernel32.INSTANCE.UnmapViewOfFile(pointer); - Kernel32.INSTANCE.CloseHandle(hMapFile); - unlinked = true; + ShmInfo info = new ShmInfo<>(); + info.size = shm_size; + info.name = nameUnmangle_TODO(shm_name); + info.handle = hMapFile; + info.pointer = pointer; + info.writePointer = writePointer; + return info; } // TODO equivalent of removing slash - private String nameUnmangle_TODO (String memoryName){ + private static String nameUnmangle_TODO (String memoryName){ return memoryName; } // TODO equivalent of adding slash // Do we need the "Local\" prefix? - private String nameMangle_TODO (String memoryName){ + private static String nameMangle_TODO (String memoryName){ // if (!memoryName.startsWith("Local" + File.separator) && !memoryName.startsWith("Global" + File.separator)) // memoryName = "Local" + File.separator + memoryName; return memoryName; @@ -258,17 +208,13 @@ private static long getSHMSize(final WinNT.HANDLE hMapFile) { return size; } + } - @Override - public String toString() { - return "ShmWindows{" + - "hMapFile=" + hMapFile + - ", size=" + size + - ", pointer=" + pointer + - ", writePointer=" + writePointer + - ", name='" + name + '\'' + - ", unlinked=" + unlinked + - '}'; - } + private static void cleanup(Pointer pointer, Pointer writePointer, WinNT.HANDLE handle) { + if (!isNull(writePointer)) Kernel32.INSTANCE.UnmapViewOfFile(writePointer); + if (!isNull(pointer)) Kernel32.INSTANCE.UnmapViewOfFile(pointer); + if (handle != null) Kernel32.INSTANCE.CloseHandle(handle); } + + private static boolean isNull(Pointer p) { return p == null || p == Pointer.NULL; } }