[GitHub] [maven-resolver] cstamas commented on a diff in pull request #299: [MRESOLVER-370] Lock factory diagnostic on failure
cstamas commented on code in PR #299:
URL: https://github.com/apache/maven-resolver/pull/299#discussion_r1227690609
##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockSupport.java:
##
@@ -32,18 +39,74 @@ public abstract class NamedLockSupport implements NamedLock
{
private final NamedLockFactorySupport factory;
+private final ConcurrentHashMap> diagnosticState; //
non-null only if diag enabled
Review Comment:
How would we list then ALL threads having locks?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #299: [MRESOLVER-370] Lock factory diagnostic on failure
cstamas commented on code in PR #299:
URL: https://github.com/apache/maven-resolver/pull/299#discussion_r1227092805
##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockSupport.java:
##
@@ -43,7 +43,7 @@ public abstract class NamedLockSupport implements NamedLock {
public NamedLockSupport(final String name, final NamedLockFactorySupport
factory) {
this.name = name;
this.factory = factory;
-this.state = factory.diagnostic ? new ConcurrentHashMap<>() : null;
+this.state = factory.isDiagnosticEnabled() ? new ConcurrentHashMap<>()
: null;
Review Comment:
Am actually quite uneasy with this "logger level changes at runtime" As
named lock instances come and go, but this is possible as well:
```
DI container
|-|
NamedLockFactory (as is singleton)
|-|
Session1
|---|
Session2
| -|
Session3
| -|
NamedLock1
|---|
NamedLock2
|-|
```
As instances are cached (and ref counted), potentially they can span
multiple sessions even, without any connection to session. IF we would always
obey logger level, we could end up with "incomplete" diag data as well, as
logger debug may be disabled-enabled-disabled-enabled, but if lock instance
spans across all 4 changes, diag map would contain only part of data
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #299: [MRESOLVER-370] Lock factory diagnostic on failure
cstamas commented on code in PR #299:
URL: https://github.com/apache/maven-resolver/pull/299#discussion_r1226661563
##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockSupport.java:
##
@@ -32,18 +38,76 @@ public abstract class NamedLockSupport implements NamedLock
{
private final NamedLockFactorySupport factory;
+private final ConcurrentHashMap> state;
+
public NamedLockSupport(final String name, final NamedLockFactorySupport
factory) {
this.name = name;
this.factory = factory;
+this.state = factory.diagnostic ? new ConcurrentHashMap<>() : null;
}
@Override
public String name() {
return name;
}
+@Override
+public boolean lockShared(long time, TimeUnit unit) throws
InterruptedException {
+if (state != null) {
+state.computeIfAbsent(Thread.currentThread(), k -> new
ArrayDeque<>())
+.push("S");
Review Comment:
Agreed, although named locks API does not limit (boxing depth), in reality
syncContext does "shallow" boxing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
