From 3b4c83ccb2b2bde11a97b7cf6fd24822122d4dff Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Wed, 17 Jul 2024 21:06:48 -0500 Subject: [PATCH] WIP: Split common Shm logic to base class There is a lot of duplicate code between ShmLinux, ShmMacOS, and ShmWindows, which can be put into an internal base class. This will make it easier to have consistent logic around the close vs. unlink behavior across platforms, without 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 | 107 ++++++++++++++++++ .../java/org/apposed/appose/shm/ShmLinux.java | 68 ++--------- .../java/org/apposed/appose/shm/ShmMacOS.java | 67 ++--------- .../org/apposed/appose/shm/ShmWindows.java | 81 ++++--------- 7 files changed, 184 insertions(+), 176 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..acee6c2 --- /dev/null +++ b/src/main/java/org/apposed/appose/shm/ShmBase.java @@ -0,0 +1,107 @@ +/*- + * #%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 { + + /** Unique name that identifies the shared memory segment. */ + private final String name; + + /** Size in bytes. */ + private final int size; + + /** Pointer referencing the shared memory. */ + private final Pointer pointer; + + /** Whether to destroy the shared memory block when {@link #close()} is called. */ + private boolean unlinkOnClose; + + /** Whether the memory block has been closed. */ + private boolean closed; + + /** Whether the memory block has been unlinked. */ + private boolean unlinked; + + protected ShmBase(final String name, final boolean create, final int size, final Pointer pointer) { + this.name = name; + this.size = size; + this.pointer = pointer; + unlinkOnClose = create; + } + + protected abstract void doClose(); + protected abstract void doUnlink(); + + @Override + public String name() { + return name; + } + + @Override + public int size() { + return size; + } + + @Override + public Pointer pointer() { + return pointer; + } + + @Override + public void unlinkOnClose(boolean unlinkOnClose) { + this.unlinkOnClose = unlinkOnClose; + } + + @Override + public synchronized void unlink() { + if (unlinked) throw new IllegalStateException("Shared memory '" + name + "' is already unlinked."); + doClose(); + doUnlink(); + unlinked = true; + } + + @Override + public synchronized void close() { + if (closed) return; + doClose(); + if (unlinkOnClose) doUnlink(); + closed = true; + } +} diff --git a/src/main/java/org/apposed/appose/shm/ShmLinux.java b/src/main/java/org/apposed/appose/shm/ShmLinux.java index 893dd0d..88d69e5 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,15 @@ 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 - */ + /** 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; - // name without leading slash private SharedMemoryLinux(final String name, final boolean create, final int size) { + super(name, create, size, pointer); + String shm_name; long prevSize; if (name == null) { @@ -130,31 +108,14 @@ private SharedMemoryLinux(final String name, final boolean create, final int siz } @Override - public String name() { - return name; - } - - @Override - public Pointer pointer() { - return pointer; - } - - @Override - public long size() { - return size; + protected void doUnlink() { + LibRtOrC.shm_unlink(name()); } - /** - * Unmap and close the shared memory. Necessary to eliminate the shared memory block - */ @Override - public synchronized void close() { - if (unlinked) { - return; - } - + protected void doClose() { // Unmap the shared memory - if (this.pointer != Pointer.NULL && LibRtOrC.munmap(this.pointer, size) == -1) { + if (pointer() != Pointer.NULL && LibRtOrC.munmap(pointer(), size()) == -1) { throw new RuntimeException("munmap failed. Errno: " + Native.getLastError()); } @@ -162,10 +123,6 @@ public synchronized void close() { 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; } /** @@ -206,14 +163,13 @@ private static long getSHMSize(final int shmFd) { public String toString() { return "ShmLinux{" + "fd=" + fd + - ", size=" + size + - ", pointer=" + pointer + - ", name='" + name + '\'' + + ", 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..1836e1f 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,10 @@ 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(name, create, size, pointer); + String shm_name; long prevSize; if (name == null) { @@ -95,57 +95,18 @@ private SharedMemoryMacOS(final String name, final boolean create, final int siz this.pointer = pointer; } - /** - * File descriptor - */ + /** 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; + protected void doUnlink() { + CLibrary.INSTANCE.shm_unlink(name()); } @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; - } - + protected void doClose() { // Unmap the shared memory - if (this.pointer != Pointer.NULL && CLibrary.INSTANCE.munmap(this.pointer, size) == -1) { + if (pointer() != Pointer.NULL && CLibrary.INSTANCE.munmap(pointer(), size()) == -1) { throw new RuntimeException("munmap failed. Errno: " + Native.getLastError()); } @@ -153,10 +114,6 @@ public synchronized void close() { 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 @@ -199,9 +156,9 @@ private static long getSHMSize(final int shmFd) { public String toString() { return "ShmMacOS{" + "fd=" + fd + - ", size=" + size + - ", pointer=" + pointer + - ", name='" + name + '\'' + + ", 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..13d379e 100644 --- a/src/main/java/org/apposed/appose/shm/ShmWindows.java +++ b/src/main/java/org/apposed/appose/shm/ShmWindows.java @@ -52,14 +52,25 @@ 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 { + + /** + * reference to the file that covers the shared memory region + */ + private WinNT.HANDLE hMapFile; + + private final Pointer writePointer; + + private String shm_name; + + private long prevSize; + private SharedMemoryWindows(final String name, final boolean create, final int size) { if(name ==null) @@ -125,62 +136,18 @@ private SharedMemoryWindows(final String name, final boolean create, final int s 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; - } + protected void doClose() { if (writePointer != null) { Kernel32.INSTANCE.UnmapViewOfFile(this.writePointer); } - Kernel32.INSTANCE.UnmapViewOfFile(pointer); + Kernel32.INSTANCE.UnmapViewOfFile(pointer()); Kernel32.INSTANCE.CloseHandle(hMapFile); - unlinked = true; + } + + @Override + protected void doUnlink() { + // TODO } // TODO equivalent of removing slash @@ -263,10 +230,10 @@ private static long getSHMSize(final WinNT.HANDLE hMapFile) { public String toString() { return "ShmWindows{" + "hMapFile=" + hMapFile + - ", size=" + size + - ", pointer=" + pointer + + ", size=" + size() + + ", pointer=" + pointer() + ", writePointer=" + writePointer + - ", name='" + name + '\'' + + ", name='" + name() + '\'' + ", unlinked=" + unlinked + '}'; }