-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MRESOLVER-499] IPC Named locks #435
Conversation
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
<description>A synchronization utility implementation using IPC.</description> | ||
|
||
<properties> | ||
<javaVersion>9</javaVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just figured: sun.misc.Signal
is unrelated to UDS...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another maintained option, Jetty have been using for long time
<groupId>com.github.jnr</groupId>
<artifactId>jnr-unixsocket</artifactId>
public static final String RESPONSE_ACQUIRE = "response-acquire"; | ||
public static final String RESPONSE_CLOSE = "response-close"; | ||
public static final String RESPONSE_STOP = "response-stop"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder wether an enum would be better?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would require rewriting the socket protocol, which is currently written to read/write a List<String>
, so definitely not an enum:
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); | |
} | |
} |
* @since 2.0.0 | ||
*/ | ||
public enum SocketFamily { | ||
inet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be uppercase. Akin to AF_INET
.
} | ||
|
||
public static SocketAddress fromString(String str) { | ||
if (str.startsWith("inet:")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this complexity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
Path lockPath = this.lockPath.toAbsolutePath().normalize(); | ||
Path lockFile = | ||
lockPath.resolve(".maven-resolver-ipc-lock-" + family.name().toLowerCase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locale.ROOT
|
||
SocketChannel createClient() throws IOException { | ||
String familyProp = System.getProperty(IpcServer.SYSTEM_PROP_FAMILY, IpcServer.DEFAULT_FAMILY); | ||
SocketFamily family = familyProp != null ? SocketFamily.valueOf(familyProp) : SocketFamily.inet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can this null
if you have a default value?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this because Windows does not use signals at all.
|
||
private static void debug(String msg, Object... args) { | ||
if (DEBUG) { | ||
System.out.printf("[ipc] [debug] " + msg + "\n", args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%n
} | ||
|
||
private static void info(String msg, Object... args) { | ||
System.out.printf("[ipc] [info] " + msg + "\n", args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%n
IPC code was lifted from @gnodet PR 😄 so will let him answer. |
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 |
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). |
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need external executables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original mvnd code did use graalvm to make native image out of server, that stuff I did not bring over, but we may...
The whole idea was to have a minimal set of dependencies so that it could easily be turned into a native executable... |
PR updated to latest Resolver master. If anyone wants to pick up, go for it. Otherwise I may pick it up later. |
This PR now updates IPC locks, added unix socket family (as module was already Java 17!), added UT, default made |
Also, changes: * native name configurable * make default nonative=true (as we do not provide any) * simplify factory
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