logdna / python

A python package for sending logs to LogDNA
MIT License
48 stars 35 forks source link

fix: Remove Thread/event to fix Django regression #107

Closed shawkinsmezmo closed 1 year ago

shawkinsmezmo commented 1 year ago

Re-opening https://github.com/logdna/python/pull/106 in order to make sure these are merged properly and separately. These were prematurely merged into a separate user branch, rather than into master.

Due to the nature in which some third party libraries / frameworks (such as Django) will instantiate and close loggers multiple times between application startup / shutdown, revert a small portion of the recent changes to return to the pattern where the flusher can be started / stopped, based on the condition of the buffer and whether or not there is buffered data. This has been tested against the previous data loss issue and is verified to still ensure that 100% of buffered data is synced.

This change can be broken down into the following functional elements:

dkhokhlov commented 1 year ago

Django may create multiple instances of this class and then try to do odd things with them, which can lead to the handler's close() method hanging while the flush thread spins endlessly, despite the fact that it is a daemon thread.

having multiple instances of LogDNAHandler is valid use case. why close() can be hanging? does not look like a "graceful" close().

shawkinsmezmo commented 1 year ago

Django may create multiple instances of this class and then try to do odd things with them, which can lead to the handler's close() method hanging while the flush thread spins endlessly, despite the fact that it is a daemon thread.

having multiple instances of LogDNAHandler is valid use case. why close() can be hanging? does not look like a "graceful" close().

That issue is resolved with this PR. The issue is a little more complicated then just that there are multiple handlers. It's not a static issue, so much as it's a state issue, and there are interesting things happening under the hood when Django instantiates loggers and other objects and then manages the life-cycles of the instances. The reason this was a problem with the previous implementation is because the previous implementation would instantiate a thread upon instantiation of the class, in the constructor, and then would shut this thread down via an event mechanism when the handler's close() method was called.

Because of what Django is doing under the hood, the handlers are instantiated and then provided as dependencies to the relevant code using Django's dependency injection. This causes issues when some of these handlers have threads that are running. Django will "hang" during startup as it is waiting for certain threads to exit, despite the fact that these threads are marked as daemon threads and thus should not cause applications to wait on their state. This may be in issue for Django and libraries like it because of object de/serialization that happens as dependencies are created and then injected as dependencies.

This PR effectively addresses this by not trying the lifecycle of a thread to class instantiation.