Add BroadcastLogger#deep_dup

[andrewn617]

Motivation / Background

This Pull Request has been created because, I want to easily duplicate instances of BroadcastLogger in my app. Just calling dup is no good because it does not duplicate the broadcasts. So I believe we could use a deep_dup method.

Detail

This Pull Request adds a ActiveSupport::BroadcastLogger#deep_dup. Calling deep_dup dups the broadcast logger, and then iterates through each of it's broadcasts and duplicates them.

Additional information

N/A

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[zzak]

Are you calling this directly or through some other API?

[zzak]

Do we care that this might drop messages?

[davidstosik]

How about just doing self.class.new(*@broadcasts.map(&:dup)?
To me, BroadcastLogger is a rather lightweight shell around the loggers, so there's not much value in duplicating it.
This way you'd avoid having to call #stop_broadcasting_to. (Also note that #stop_broadcasting_to is equivalent to removing an item from @broadcasts.

[davidstosik]

Ah, I just realized that calling dup like you do is not going to work. Even @broadcasts itself is not duplicated:

bl1 = ActiveSupport::BroadcastLogger.new
bl2 = bl1.dup

bl1.broadcasts.__id__ == bl2.broadcasts.__id__
#=> true

One more reason to use self.class.new instead. 😉

[davidstosik]

Considering my other comments, I'd suggest adding assertions that confirm the original BroadcastLogger (and its list of @broadcasts) was not altered, and is not tied to its duplicate.

assert_equal logger, broadcast_logger.broadcasts.sole

[davidstosik]

I just tried it and got this error:

Failure:
ActiveSupport::BroadcastLoggerTest#test_#deep_dup_duplicates_the_broadcasts [activesupport/test/broadcast_logger_test.rb:300]:
No visible difference in the ActiveSupport::BroadcastLoggerTest::CustomLogger#inspect output.
You should look at the implementation of #== on ActiveSupport::BroadcastLoggerTest::CustomLogger or its members.
#<ActiveSupport::BroadcastLoggerTest::CustomLogger:0xXXXXXX @adds=[], @closed=false, @chevrons=[], @level=2, @local_level=0, @progname=nil, @formatter=nil>

Very curious. Looking at the doc for MiniTest's assert_equal, I found this:

If there is no visible difference but the assertion fails, you should suspect that your #== is buggy, or your inspect output is missing crucial details.

Instead I tried using assert_same:

      assert_same logger, broadcast_logger.broadcasts.sole
      refute_same logger, duplicate.broadcasts.sole
      assert_equal ::Logger::WARN, duplicate.broadcasts.sole.level

And then I get the failure I expected:

Failure:
ActiveSupport::BroadcastLoggerTest#test_#deep_dup_duplicates_the_broadcasts [activesupport/test/broadcast_logger_test.rb:300]:
Expected #<ActiveSupport::BroadcastLoggerTest::CustomLogger:0x00000001030de158 @adds=[], @closed=false, @chevrons=[], @level=2, @local_level=0, @progname=nil, @formatter=nil> (oid=5640) to be the same as #<ActiveSupport::BroadcastLoggerTest::CustomLogger:0x00000001030de1f8 @adds=[], @closed=false, @chevrons=[], @level=2, @local_level=0, @progname=nil, @formatter=nil> (oid=5660).

[davidstosik]

Here are the changes that I would suggest to make:

diff --git i/activesupport/lib/active_support/broadcast_logger.rb w/activesupport/lib/active_support/broadcast_logger.rb
index 9454c456b4..605f275736 100644
--- i/activesupport/lib/active_support/broadcast_logger.rb
+++ w/activesupport/lib/active_support/broadcast_logger.rb
@@ -219,14 +219,10 @@ def fatal!
     end
 
     def deep_dup
-      duplicate = dup
-
-      broadcasts.each do |broadcast|
-        duplicate.stop_broadcasting_to(broadcast)
-        duplicate.broadcast_to(broadcast.dup)
+      self.class.new(*@broadcasts.map(&:dup)).tap do |logger|
+        logger.formatter = formatter.dup
+        logger.progname = progname.dup
       end
-
-      duplicate
     end
 
     private
diff --git i/activesupport/test/broadcast_logger_test.rb w/activesupport/test/broadcast_logger_test.rb
index d018e79018..703bb445cf 100644
--- i/activesupport/test/broadcast_logger_test.rb
+++ w/activesupport/test/broadcast_logger_test.rb
@@ -297,8 +297,9 @@ def info(msg, &block)
 
       duplicate = broadcast_logger.deep_dup
 
+      assert_same logger, broadcast_logger.broadcasts.sole
+      refute_same logger, duplicate.broadcasts.sole
       assert_equal ::Logger::WARN, duplicate.broadcasts.sole.level
-      assert_not_equal logger, duplicate.broadcasts.sole
     end
 
     class CustomLogger

One more thing: using #deep_dup instead of #dup is "opt-in", and developers could just miss that they need to use it.
Should #dup warn the developer that it might not be doing what they expect? (Or should this PR override #dup instead of implementing #deep_dup?)

[jhawthorn]

I agree with @davidstosik's suggestion. Unless there's a reason users might want a "shallow" dup, it would be best that dup copied the @broadcasts as well.

This can be best done by defining initialize_copy.

[davidstosik]

Late nitpick, but I don't get why this was better than doing @broadcasts = other.broadcasts.map(&:dup) in the first line. 🤔