[MRESOLVER-499] IPC Named locks #435

cstamas · 2024-02-22T15:53:15Z

Implementation of IPC Named Locks. Code originates from mvnd, this was the original locking it used, adapted to NamedLocks, that now became possible.

https://issues.apache.org/jira/browse/MRESOLVER-499

Implementation of IPC Named Locks. Code originates from mvnd, this was the original locking it used, adapted to NamedLocks, that now became possible. --- https://issues.apache.org/jira/browse/MRESOLVER-499

cstamas · 2024-02-22T18:38:29Z

maven-resolver-named-locks-ipc/pom.xml

+  <description>A synchronization utility implementation using IPC.</description>
+
+  <properties>
+    <javaVersion>9</javaVersion>


This is due sun.misc.Signal stuff, any ideas? If release=8 it does not compile.
Or, if we are fine, we should go 16 and add UNIX domain sockets :)

UDS in 16 was incomplete, that was fixed in 17. A great alternative is rather https://github.com/kohlschutter/junixsocket. Would consider that one to reach a broader audience.

You are right: we should go for 17 then, basically reduce our "resolution" to LTS versions only. The Artifact Generator PR uses UDS and is currently 16... This one has non trivial dependency trail (and native stuff).

Also, just figured: sun.misc.Signal is unrelated to UDS...

Another maintained option, Jetty have been using for long time

<groupId>com.github.jnr</groupId> <artifactId>jnr-unixsocket</artifactId>

michael-o · 2024-02-22T20:32:53Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcMessages.java

+    public static final String RESPONSE_ACQUIRE = "response-acquire";
+    public static final String RESPONSE_CLOSE = "response-close";
+    public static final String RESPONSE_STOP = "response-stop";
+}


I wonder wether an enum would be better?!

It would require rewriting the socket protocol, which is currently written to read/write a List<String>, so definitely not an enum:

maven-resolver/maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java

Lines 266 to 308 in 814b673

void receive() {

try {

while (true) {

int id = input.readInt();

int sz = input.readInt();

List<String> s = new ArrayList<>(sz);

for (int i = 0; i < sz; i++) {

s.add(input.readUTF());

}

CompletableFuture<List<String>> f = responses.remove(id);

if (f == null || s.isEmpty()) {

throw new IllegalStateException("Protocol error");

}

f.complete(s);

}

} catch (EOFException e) {

// server is stopped; just quit

} catch (Exception e) {

close(e);

}

}

List<String> send(List<String> request, long time, TimeUnit unit) throws TimeoutException, IOException {

ensureInitialized();

int id = requestId.incrementAndGet();

CompletableFuture<List<String>> response = new CompletableFuture<>();

responses.put(id, response);

synchronized (output) {

output.writeInt(id);

output.writeInt(request.size());

for (String s : request) {

output.writeUTF(s);

}

output.flush();

}

try {

return response.get(time, unit);

} catch (InterruptedException e) {

throw (IOException) new InterruptedIOException("Interrupted").initCause(e);

} catch (ExecutionException e) {

throw new IOException("Execution error", e);

}

}

michael-o · 2024-02-22T20:36:39Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/SocketFamily.java

+ * @since 2.0.0
+ */
+public enum SocketFamily {
+    inet;


Should be uppercase. Akin to AF_INET.

michael-o · 2024-02-22T20:38:05Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/SocketFamily.java

+    }
+
+    public static SocketAddress fromString(String str) {
+        if (str.startsWith("inet:")) {


Why this complexity?

Because UDS was supported in the original version:
https://github.com/apache/maven-mvnd/blob/0.7.1/common/src/main/java/org/mvndaemon/mvnd/common/SocketFamily.java

Maybe adding the UDS support and the unit test would help: https://github.com/apache/maven-mvnd/blob/0.7.1/common/src/test/java/org/mvndaemon/mvnd/common/SocketFamilyTest.java

michael-o · 2024-02-22T20:42:28Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java

+
+        Path lockPath = this.lockPath.toAbsolutePath().normalize();
+        Path lockFile =
+                lockPath.resolve(".maven-resolver-ipc-lock-" + family.name().toLowerCase());


Locale.ROOT

michael-o · 2024-02-22T20:43:12Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java

+
+    SocketChannel createClient() throws IOException {
+        String familyProp = System.getProperty(IpcServer.SYSTEM_PROP_FAMILY, IpcServer.DEFAULT_FAMILY);
+        SocketFamily family = familyProp != null ? SocketFamily.valueOf(familyProp) : SocketFamily.inet;


How can this null if you have a default value?

michael-o · 2024-02-22T20:48:05Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcServer.java

+        try {
+            sun.misc.Signal.handle(new sun.misc.Signal("INT"), sun.misc.SignalHandler.SIG_IGN);
+            if (IpcClient.IS_WINDOWS) {
+                sun.misc.Signal.handle(new sun.misc.Signal("TSTP"), sun.misc.SignalHandler.SIG_IGN);


I don't understand this because Windows does not use signals at all.

michael-o · 2024-02-22T20:48:45Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcServer.java

+
+    private static void debug(String msg, Object... args) {
+        if (DEBUG) {
+            System.out.printf("[ipc] [debug] " + msg + "\n", args);


michael-o · 2024-02-22T20:48:53Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcServer.java

+    }
+
+    private static void info(String msg, Object... args) {
+        System.out.printf("[ipc] [info] " + msg + "\n", args);


cstamas · 2024-02-22T20:50:55Z

IPC code was lifted from @gnodet PR 😄 so will let him answer.

olamy · 2024-02-22T22:37:12Z

just saying in case you don't want to reinvent the wheel. You can server/client unixsocket already done in Jetty https://github.com/jetty/jetty.project/tree/jetty-10.0.x/jetty-unixsocket

cstamas · 2024-02-23T20:47:04Z

There is a mixup here: UDS is NOT used here at all, it is used by other PR, the Signer to communicate with gpg-agent. This PR does not use UDS, it uses plain inet sockets, which IMO is okay, as that could mean that Server could be started even on remote host (did not test, just guessing).

slawekjaranowski · 2024-02-26T21:08:13Z

maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java

+                ServerSocketChannel ss = family.openServerSocket();
+                String tmpaddr = SocketFamily.toString(ss.getLocalAddress());
+                String rand = Long.toHexString(new Random().nextLong());
+                String syncCmd = IS_WINDOWS ? "mvnd-sync.exe" : "mvnd-sync";


do we need external executables?

Original mvnd code did use graalvm to make native image out of server, that stuff I did not bring over, but we may...

gnodet · 2024-02-27T23:49:42Z

just saying in case you don't want to reinvent the wheel. You can server/client unixsocket already done in Jetty https://github.com/jetty/jetty.project/tree/jetty-10.0.x/jetty-unixsocket

The whole idea was to have a minimal set of dependencies so that it could easily be turned into a native executable...

[MRESOLVER-499] IPC Named locks

814b673

Implementation of IPC Named Locks. Code originates from mvnd, this was the original locking it used, adapted to NamedLocks, that now became possible. --- https://issues.apache.org/jira/browse/MRESOLVER-499

cstamas requested review from gnodet, michael-o and slawekjaranowski February 22, 2024 15:53

cstamas self-assigned this Feb 22, 2024

cstamas mentioned this pull request Feb 22, 2024

[MRESOLVER-499] IPC Named Locks #433

Closed

cstamas commented Feb 22, 2024

View reviewed changes

michael-o requested changes Feb 22, 2024

View reviewed changes

slawekjaranowski reviewed Feb 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRESOLVER-499] IPC Named locks #435

[MRESOLVER-499] IPC Named locks #435

cstamas commented Feb 22, 2024

cstamas Feb 22, 2024 •

edited

michael-o Feb 22, 2024

cstamas Feb 22, 2024

cstamas Feb 22, 2024

olamy Feb 22, 2024

michael-o Feb 22, 2024

gnodet Feb 27, 2024

michael-o Feb 22, 2024

michael-o Feb 22, 2024

gnodet Feb 27, 2024

gnodet Feb 27, 2024

michael-o Feb 22, 2024

michael-o Feb 22, 2024

michael-o Feb 22, 2024

michael-o Feb 22, 2024

michael-o Feb 22, 2024

cstamas commented Feb 22, 2024

olamy commented Feb 22, 2024

cstamas commented Feb 23, 2024

slawekjaranowski Feb 26, 2024

cstamas Feb 27, 2024

gnodet commented Feb 27, 2024

	void receive() {
	try {
	while (true) {
	int id = input.readInt();
	int sz = input.readInt();
	List<String> s = new ArrayList<>(sz);
	for (int i = 0; i < sz; i++) {
	s.add(input.readUTF());
	}
	CompletableFuture<List<String>> f = responses.remove(id);
	if (f == null \|\| s.isEmpty()) {
	throw new IllegalStateException("Protocol error");
	}
	f.complete(s);
	}
	} catch (EOFException e) {
	// server is stopped; just quit
	} catch (Exception e) {
	close(e);
	}
	}

	List<String> send(List<String> request, long time, TimeUnit unit) throws TimeoutException, IOException {
	ensureInitialized();
	int id = requestId.incrementAndGet();
	CompletableFuture<List<String>> response = new CompletableFuture<>();
	responses.put(id, response);
	synchronized (output) {
	output.writeInt(id);
	output.writeInt(request.size());
	for (String s : request) {
	output.writeUTF(s);
	}
	output.flush();
	}
	try {
	return response.get(time, unit);
	} catch (InterruptedException e) {
	throw (IOException) new InterruptedIOException("Interrupted").initCause(e);
	} catch (ExecutionException e) {
	throw new IOException("Execution error", e);
	}
	}

[MRESOLVER-499] IPC Named locks #435

Are you sure you want to change the base?

[MRESOLVER-499] IPC Named locks #435

Conversation

cstamas commented Feb 22, 2024

cstamas Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Feb 22, 2024

olamy commented Feb 22, 2024

cstamas commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet commented Feb 27, 2024

cstamas Feb 22, 2024 •

edited