scwhittle commented on pull request #12825: URL: https://github.com/apache/beam/pull/12825#issuecomment-692275145
@lukecwik I think to have a single lock, we would need to use the PrintStream lock itself, as that is what is synchronized on by Throwable.printStackTrace and which can be synchronize outside and inside other synchronization blocks if we choose another lock to guard the buffer, ErrorManager, handler. However using the PrintStream as a lock within the DataflowWorkerLoggingHandler seemed perhaps error prone as that extends a Handler which might have it's own synchronized methods (on itself not the PrintStream) or callers that synchronize on the Handler (and not on the PrintStream). Additionally it seemed a bit gross since there is not always a PrintStream for a handler, though we could just allow injecting a lock Object into the handler to hide that. If you think that approach is still preferrable, I can put that together. Regarding on why to synchronize on buffer, that allows us to keep the preconditioncheck to sanity check our implementation. But that seems overkill, so I will just remove 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: [email protected]
