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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
