FluentLogFactory: Why use WeakHashMap to store loggers?

[todesking]

        FluentLoggerFactory factory = new FluentLoggerFactory();
        ArrayList<FluentLogger> loggers = new ArrayList<FluentLogger>();
        for(int i=0; i<100; i++) {
            loggers.add(factory.getLogger("testtag" + i, "localhost", 999));
        }
        System.gc();
        assertEquals(loggers.size(), factory.getLoggers().size());

The last assertion is sometimes failed. That because WeakHashMap has weak-referenced keys and strong-referenced values, so when key(String generated in FluentLoggerFactory#getLogger()) is GCed, correspond logger is removed from the map.

Because of this, FluentLogerFactory#closeAll() and flushAll() has unexpected behaviour: Some loggers are closed/flushed, and rest is not touched.

It seems just a bug and use normal HashMap to fix it. Is this correct?

[komamitsu]

@todesking Thanks for your report.

@muga Why did you change the implementation of Map from HashMap to WeakHashMap at d5efd4d?

[komamitsu]

@muga Any updates?

[muga]

@todesking We've used WeakHashMap and finalize method for reducing the heap space consumption. I thought and considered the possibility that some users create a lot of FluentLogger objects on his JVM. But the implementation is not good in some cases, you know.

@komamitsu We should remove FluentLoggerFactory (and the WeakHashMap impl.) from external API. FluentLogger objects and the object creations should be managed by users.

[oza]

@todesking good catch, thanks for your report.
We have 2 options:

1. Reverting the store of FluentLoggerFactory to HashMap.
2. Removing FluentLoggerFactory.

I prefer to use 1. for the backward compatibility. Also, users can choose whether they manage the object by themselves or by FluentLoggerFactory. For handling small number of the objects, it's enough.

[komamitsu]

@muga @oza Thanks for your ideas.

For option 1, I'm concerned about memory consumption because all FluentLoggers stored in HashMap are never released. I think option 2 is simple and isn't confusable. Of course, backward compatibility is important, though as you said.

Also, users can choose whether they manage the object by themselves or by FluentLoggerFactory.

FluentLogger internally depends on FluentLogFactory. https://github.com/fluent/fluent-logger-java/blob/master/src/main/java/org/fluentd/logger/FluentLogger.java#L32. So we need to remove the dependency from FluentLogger if we choose this option.

[oza]

@komamitsu Rethinking of this - how about having the key of WeakHashMap at FluentLogger via FluentLogger#setFactoryKey or something to prevent the GC? When a instance of FluentLogger#close()
is called, resource management can be done by setting key as null.

[komamitsu]

@oza Make sense.
But adding another attribute into FluentLogger may make it more complicated. Also, factoryKey doesn't sound an essential information for FluentLogger...

I think the cause of this problem is the way of holding a reference of FluentLogger in FluentLoggerFactory. I created a PR (#38) for it. The PR has the following little downsides. But I think they are ignorable.

• FluentLoggerFactory#getLogger linearly searches for a FluentLogger which has the same key consists of tagPrefix, host, port and so on. But the number of FluentLogger entries won't be large actually.
• FluentLoggerFactory#getLoggers broke the compatibility. But this method is package private and only for test.

What do you think?

[oza]

@komamitsu I agree with your idea. @muga what do you think?

[muga]

@komamitsu @oza thank you for driving this issue and fixing. #38 looks good to me to fix this issue. compatibility is important:-)

[oza]

@muga @komamitsu Merged #38.

@todesking Thanks for your reporting!