vp340 commented on code in PR #3521: URL: https://github.com/apache/cxf/pull/3521#discussion_r4174319742
########## rt/features/logging/src/main/java/org/apache/cxf/ext/logging/LoggingOutInterceptor.java: ########## Review Comment: Hi @reta, We are on the same page on that :) . If fact the first thought that I have was also to unregister the callback from the cos (see first commit of this PR https://github.com/apache/cxf/pull/3521/changes/2b482dccf81f8a4ae726f82b79119d34cbb7263d ). Then when I ran the tests, half failed due to java.util.ConcurrentModificationException . This happens because the cb.onClose() is called inside a cycle of the callbacks list itself. `for (CachedOutputStreamCallback cb : callbacks) { try { cb.onClose(this); ` So obviously we can't just deregister the cb from the list (that under the hood does callbacks.remove(cb) as we would modify the list while the for loop is still cycling it. I introduced the OneTimeLoggingCallback to resolve the problem in the feature logging module in order not to touch the core module. But if U wan't to pursue the cos.deregisterCallback(this); solution (that in my opinion is more elegant ) , we have to cycle a snapshot of the callbacks and not the real list. I prepare this alternative solution in https://github.com/apache/cxf/pull/3535 . I also add the check in the test to ensure that che callback is deregister from the cos. Let me know which solution do U prefer ;) Have a good evening! -- 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]
