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 2021/02/25 16:19:45 UTC

[GitHub] [logging-log4j2] jkozlowski opened a new pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

jkozlowski opened a new pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470


   


----------------------------------------------------------------
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 #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#discussion_r583283312



##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamManager.java
##########
@@ -278,10 +278,13 @@ protected synchronized void flushDestination() {
      */
     protected synchronized void flushBuffer(final ByteBuffer buf) {
         ((Buffer) buf).flip();

Review comment:
       So previously in the exceptional case, we would fail out before calling clear, leaving ourselves with buffer position zero, limit N (potentially a small fraction of the capacity, based on potentially a single event).
   Subsequent attempts to flush the buffer can only further reduce the available buffer space until our limit and position converge. At that point our `buf.remaining() > 0` condition can never succeed if I'm reasoning through this correctly, and we cannot write any data.
   Great catch!




----------------------------------------------------------------
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 #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-787966215


   It looks like a build flake, I'll take care of the merge in the next hour


----------------------------------------------------------------
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] vy commented on pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
vy commented on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-787799822


   @carterkozak, I am joining to the party late, sorry. What is the status of this PR? Can we merge it?


----------------------------------------------------------------
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] vy edited a comment on pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
vy edited a comment on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-787799822


   @carterkozak, I am joining to the party late, sorry. What is the status of this PR? Can we merge it? (Note that the volunteer to merge might need to port this to `master` too.)


----------------------------------------------------------------
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] jkozlowski commented on pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
jkozlowski commented on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-787941498


   Hmm, there were merge conflicts, so I merged develop and now builds aren't green; I wonder if it's an actual issue or a flake?


----------------------------------------------------------------
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] jkozlowski commented on pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
jkozlowski commented on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-787872163


   @vy should be done now


----------------------------------------------------------------
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 #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-788031443


   @vy Yep, I've taken care of merging 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] vy commented on pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
vy commented on pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470#issuecomment-787803000


   @jkozlowski, thanks so much for the contribution! Would you mind updating the `changes.xml` too, please? That is,
   
   ```xml
   <action issue="LOG4J2-3028" dev="carterkozak" type="fix" due-to="Jakub Kozlowski">
     OutputStreamManager does not clear buffer on exception.
   </action>
   ```


----------------------------------------------------------------
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 closed pull request #470: [LOG4J2-3028] OutputStreamManager#flushBuffer always clears the buffer.

Posted by GitBox <gi...@apache.org>.
carterkozak closed pull request #470:
URL: https://github.com/apache/logging-log4j2/pull/470


   


----------------------------------------------------------------
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