You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/05/26 13:22:49 UTC
[GitHub] [geode] jvarenina opened a new pull request, #7725: GEODE-10338: Fix LogWriterAppender shutdown
jvarenina opened a new pull request, #7725:
URL: https://github.com/apache/geode/pull/7725
When a stop session is called on the LogWriterAppender, it closes the
ManagerLogWriter's files. Still, it does not release
ManagerLogWriter's reference, so the LogWriterAppender instance is
kept around after disconnect. This situation ends up keeping the
InternalDistributedSystem alive.
The fix is to clear the LogWriterAppender's reference to the
ManagerLogWriter when the appender session is stopped.
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a diff in pull request #7725: GEODE-10338: Fix LogWriterAppender shutdown
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7725:
URL: https://github.com/apache/geode/pull/7725#discussion_r888481928
##########
geode-log4j/src/main/java/org/apache/geode/logging/log4j/internal/impl/LogWriterAppender.java:
##########
@@ -302,10 +302,15 @@ public synchronized void startSession() {
@Override
public synchronized void stopSession() {
+ if (logWriter == null) {
+ pause();
+ return;
+ }
Review Comment:
I think you should just remove the early-out. In theory, an early-out or an assert could be added other places as well such as startSession() but it's better to be consistent in this case. The usage of LogWriterAppender is driven by the LoggingSession code and nothing will ever call startSession or stopSession without calling createSession. If it was a user-facing User API then I might be inclined to protected these method calls from a null logWriter. Even if we did add these protect clauses then I would think that we should ensure that all log statements (such as line 309) print out.
##########
geode-log4j/src/main/java/org/apache/geode/logging/log4j/internal/impl/LogWriterAppender.java:
##########
@@ -302,10 +302,15 @@ public synchronized void startSession() {
@Override
public synchronized void stopSession() {
+ if (logWriter == null) {
+ pause();
+ return;
+ }
LOGGER.info("Stopping session in {}.", this);
logWriter.shuttingDown();
pause();
logWriter.closingLogFile();
+ logWriter = null;
Review Comment:
I think it would be safer to place `logWriter = null;` within a finally block just in case one of those logWriter calls above throws something like an IOException or a nested IOException.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jvarenina commented on pull request #7725: GEODE-10338: Fix LogWriterAppender shutdown
Posted by GitBox <gi...@apache.org>.
jvarenina commented on PR #7725:
URL: https://github.com/apache/geode/pull/7725#issuecomment-1156398017
When created in the same VM, all cache instances use the same LogWriterAppender service. Therefore all tests that create multiple caches and try to close them will fail with `NullpointerExecption `(the `stopSession()` method will be executed more than once for the same `LogWriterAppender`).
For example check MultipleCacheJUnitTest.cacheCreateTwoPeerCaches test case:
```
Jale create cache1
Jale LOGWRITER with instance org.apache.geode.logging.log4j.internal.impl.LogWriterAppender@49925e47:LOGWRITER {eagerMemberName=null, lazyMemberName=appendLog=true, security=false, paused=false, loggingSessionRegistry=org.apache.geode.logging.internal.LoggingSessionRegistryProvider@4ae90315, logWriter=org.apache.geode.logging.log4j.internal.impl.NullLogWriter@6b0572fa, debug=false} has logWriter object: org.apache.geode.logging.log4j.internal.impl.NullLogWriter@6b0572fa
Jale SECURITYLOGWRITER with instance org.apache.geode.logging.log4j.internal.impl.LogWriterAppender@7ed478ab:SECURITYLOGWRITER {eagerMemberName=null, lazyMemberName=appendLog=true, security=true, paused=false, loggingSessionRegistry=org.apache.geode.logging.internal.LoggingSessionRegistryProvider@4ae90315, logWriter=org.apache.geode.logging.log4j.internal.impl.NullLogWriter@3a981478, debug=false} has logWriter object: org.apache.geode.logging.log4j.internal.impl.NullLogWriter@3a981478
Jale create cache2
Jale LOGWRITER with instance org.apache.geode.logging.log4j.internal.impl.LogWriterAppender@49925e47:LOGWRITER {eagerMemberName=null, lazyMemberName=appendLog=true, security=false, paused=false, loggingSessionRegistry=org.apache.geode.logging.internal.LoggingSessionRegistryProvider@4ae90315, logWriter=org.apache.geode.logging.log4j.internal.impl.NullLogWriter@78cc6db3, debug=false} has logWriter object: org.apache.geode.logging.log4j.internal.impl.NullLogWriter@78cc6db3
Jale SECURITYLOGWRITER with instance org.apache.geode.logging.log4j.internal.impl.LogWriterAppender@7ed478ab:SECURITYLOGWRITER {eagerMemberName=null, lazyMemberName=appendLog=true, security=true, paused=false, loggingSessionRegistry=org.apache.geode.logging.internal.LoggingSessionRegistryProvider@4ae90315, logWriter=org.apache.geode.logging.log4j.internal.impl.NullLogWriter@3839a6d9, debug=false} has logWriter object: org.apache.geode.logging.log4j.internal.impl.NullLogWriter@3839a6d9
Jale closing: cache1
Jale stopSession: org.apache.geode.logging.log4j.internal.impl.NullLogWriter@78cc6db3
Jale stopSession: org.apache.geode.logging.log4j.internal.impl.NullLogWriter@3839a6d9
Jale closing: cache2
Jale stopSession: null
java.lang.NullPointerException
```
I think it would be best to return the check whether `logWriter` is null at the beginning of `stopSession()` method for these test cases to work. @kirklund, what do you think?
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac merged pull request #7725: GEODE-10338: Fix LogWriterAppender shutdown
Posted by GitBox <gi...@apache.org>.
mivanac merged PR #7725:
URL: https://github.com/apache/geode/pull/7725
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org