You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/10/30 02:53:06 UTC
[GitHub] [logging-log4j2] henryptung opened a new pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
henryptung opened a new pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434
Make sure the returned Cancellable handler contains a strong
reference to the callback, so that it is not garbage-collected
prematurely. Add Javadoc note about collection semantics.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434#issuecomment-974773891
@carterkozak Are you doing something with this?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434#discussion_r516721169
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/util/DefaultShutdownCallbackRegistry.java
##########
@@ -87,42 +92,41 @@ public void run() {
}
private static class RegisteredCancellable implements Cancellable {
- // use a reference to prevent memory leaks
- private final Reference<Runnable> hook;
- private Collection<Cancellable> registered;
+ private Runnable callback;
+ private Collection<Reference<Cancellable>> registered;
- RegisteredCancellable(final Runnable callback, final Collection<Cancellable> registered) {
+ RegisteredCancellable(final Runnable callback, final Collection<Reference<Cancellable>> registered) {
+ this.callback = callback;
this.registered = registered;
- hook = new SoftReference<>(callback);
}
@Override
public void cancel() {
- hook.clear();
- registered.remove(this);
+ callback = null;
+ registered.removeIf(ref -> ref.get() == this);
Review comment:
I'm going to update this to include the null check to remove collected values, and add a null check on registered so that subsequent `cancel()` invocations do not NPE.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434#discussion_r516721169
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/util/DefaultShutdownCallbackRegistry.java
##########
@@ -87,42 +92,41 @@ public void run() {
}
private static class RegisteredCancellable implements Cancellable {
- // use a reference to prevent memory leaks
- private final Reference<Runnable> hook;
- private Collection<Cancellable> registered;
+ private Runnable callback;
+ private Collection<Reference<Cancellable>> registered;
- RegisteredCancellable(final Runnable callback, final Collection<Cancellable> registered) {
+ RegisteredCancellable(final Runnable callback, final Collection<Reference<Cancellable>> registered) {
+ this.callback = callback;
this.registered = registered;
- hook = new SoftReference<>(callback);
}
@Override
public void cancel() {
- hook.clear();
- registered.remove(this);
+ callback = null;
+ registered.removeIf(ref -> ref.get() == this);
Review comment:
I'm going to update this to include the null check to remove collected values, and add a null check on `registered` so that subsequent `cancel()` invocations do not NPE.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434#issuecomment-974773891
@carterkozak Are you doing something with this?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434#issuecomment-720538814
I think the difference between the first and second commit on this PR is whether or not the JeroMqManager class (and related context classes) should be allowed to be unloaded if they're only referenced by the shutdown hook. I'm' not sure whether or not that's important.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #434: LOG4J2-2954 Retain strong reference to shutdown callbacks
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #434:
URL: https://github.com/apache/logging-log4j2/pull/434#issuecomment-721158932
I think the approach in the first commit is less risky, so I'm going to merge that after I verify tests pass locally to ensure this makes the 2.14.0 release.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org