From a167be558068618e52252d49e5d865a302c0fb9d Mon Sep 17 00:00:00 2001 From: Petr Levtonov Date: Sun, 17 Jan 2021 15:54:58 +0100 Subject: [PATCH] facade/ignition#302: Fix memory leaks caused by log and query recorder - Add optional log recorder limit to prevent memory leaks. Especially noticable during long running tasks or daemons. - Rewrote query recorder to not tigthly couple it with the framework container when deciding whether to include bindings or limit the maximum query count that is recorded. - Added a config flag to disable log recording all together. - Made sure that query and log recorders are not used when their enable flag is disabled. --- config/flare.php | 2 + src/IgnitionServiceProvider.php | 76 +++++++++++++++-------- src/LogRecorder/LogRecorder.php | 22 ++++++- src/QueryRecorder/QueryRecorder.php | 49 ++++++++++++--- tests/LogRecorder/LogRecorderTest.php | 71 +++++++++++++++++++++ tests/QueryRecorder/QueryRecorderTest.php | 47 +++++++++++++- 6 files changed, 231 insertions(+), 36 deletions(-) create mode 100644 tests/LogRecorder/LogRecorderTest.php diff --git a/config/flare.php b/config/flare.php index d91e8208..c2773883 100644 --- a/config/flare.php +++ b/config/flare.php @@ -32,6 +32,8 @@ 'report_query_bindings' => true, 'report_view_data' => true, 'grouping_type' => null, + 'report_logs' => true, + 'maximum_number_of_collected_logs' => 200, ], /* diff --git a/src/IgnitionServiceProvider.php b/src/IgnitionServiceProvider.php index 19ec261d..a5098495 100644 --- a/src/IgnitionServiceProvider.php +++ b/src/IgnitionServiceProvider.php @@ -90,8 +90,14 @@ public function boot() $this->setupQueue($this->app->get('queue')); } - $this->app->make(QueryRecorder::class)->register(); - $this->app->make(LogRecorder::class)->register(); + if (config('flare.reporting.report_logs')) { + $this->app->make(LogRecorder::class)->register(); + } + + if (config('flare.reporting.report_queries')) { + $this->app->make(QueryRecorder::class)->register(); + } + $this->app->make(DumpRecorder::class)->register(); } @@ -106,9 +112,12 @@ public function register() ->registerWhoopsHandler() ->registerIgnitionConfig() ->registerFlare() - ->registerLogRecorder() ->registerDumpCollector(); + if (config('flare.reporting.report_logs')) { + $this->registerLogRecorder(); + } + if (config('flare.reporting.report_queries')) { $this->registerQueryRecorder(); } @@ -279,13 +288,14 @@ protected function getLogLevel(string $logLevelString): int return $logLevel; } - protected function registerLogRecorder() + protected function registerLogRecorder(): self { - $logCollector = $this->app->make(LogRecorder::class); - - $this->app->singleton(LogRecorder::class); - - $this->app->instance(LogRecorder::class, $logCollector); + $this->app->singleton(LogRecorder::class, function (Application $app): LogRecorder { + return new LogRecorder( + $app, + $app->get('config')->get('flare.reporting.maximum_number_of_collected_logs') + ); + }); return $this; } @@ -319,30 +329,42 @@ protected function registerCommands() return $this; } - protected function registerQueryRecorder() + protected function registerQueryRecorder(): self { - $queryCollector = $this->app->make(QueryRecorder::class); - - $this->app->singleton(QueryRecorder::class); - - $this->app->instance(QueryRecorder::class, $queryCollector); + $this->app->singleton(QueryRecorder::class, function (Application $app): QueryRecorder { + return new QueryRecorder( + $app, + $app->get('config')->get('flare.reporting.report_query_bindings'), + $app->get('config')->get('flare.reporting.maximum_number_of_collected_logs') + ); + }); return $this; } protected function registerBuiltInMiddleware() { - $middleware = collect([ + $middlewares = [ SetNotifierName::class, AddEnvironmentInformation::class, - AddLogs::class, - AddDumps::class, - AddQueries::class, - AddSolutions::class, - ]) - ->map(function (string $middlewareClass) { - return $this->app->make($middlewareClass); - }); + ]; + + if (config('flare.reporting.report_logs')) { + $middlewares[] = AddLogs::class; + } + + $middlewares[] = AddDumps::class; + + if (config('flare.reporting.report_queries')) { + $middlewares[] = AddQueries::class; + } + + $middlewares[] = AddSolutions::class; + + $middleware = collect($middlewares) + ->map(function (string $middlewareClass) { + return $this->app->make($middlewareClass); + }); if (config('flare.reporting.collect_git_information')) { $middleware[] = (new AddGitInformation()); @@ -436,12 +458,14 @@ protected function setupQueue(QueueManager $queue) $queue->looping(function () { $this->app->get(Flare::class)->reset(); + if (config('flare.reporting.report_logs')) { + $this->app->make(LogRecorder::class)->reset(); + } + if (config('flare.reporting.report_queries')) { $this->app->make(QueryRecorder::class)->reset(); } - $this->app->make(LogRecorder::class)->reset(); - $this->app->make(DumpRecorder::class)->reset(); }); } diff --git a/src/LogRecorder/LogRecorder.php b/src/LogRecorder/LogRecorder.php index 543aa46d..31b3706e 100644 --- a/src/LogRecorder/LogRecorder.php +++ b/src/LogRecorder/LogRecorder.php @@ -14,9 +14,13 @@ class LogRecorder /** @var \Illuminate\Contracts\Foundation\Application */ protected $app; - public function __construct(Application $app) + /** @var int|null */ + private $maxLogs; + + public function __construct(Application $app, ?int $maxLogs = null) { $this->app = $app; + $this->maxLogs = $maxLogs; } public function register(): self @@ -33,6 +37,10 @@ public function record(MessageLogged $event): void } $this->logMessages[] = LogMessage::fromMessageLoggedEvent($event); + + if (is_int($this->maxLogs)) { + $this->logMessages = array_slice($this->logMessages, -$this->maxLogs); + } } public function getLogMessages(): array @@ -68,4 +76,16 @@ public function reset(): void { $this->logMessages = []; } + + public function getMaxLogs(): ?int + { + return $this->maxLogs; + } + + public function setMaxLogs(?int $maxLogs): self + { + $this->maxLogs = $maxLogs; + + return $this; + } } diff --git a/src/QueryRecorder/QueryRecorder.php b/src/QueryRecorder/QueryRecorder.php index ad14fb6d..423d67c7 100644 --- a/src/QueryRecorder/QueryRecorder.php +++ b/src/QueryRecorder/QueryRecorder.php @@ -13,9 +13,20 @@ class QueryRecorder /** @var \Illuminate\Contracts\Foundation\Application */ protected $app; - public function __construct(Application $app) - { + /** @var bool */ + private $reportBindings; + + /** @var int|null */ + private $maxQueries; + + public function __construct( + Application $app, + bool $reportBindings = true, + ?int $maxQueries = null + ) { $this->app = $app; + $this->reportBindings = $reportBindings; + $this->maxQueries = $maxQueries; } public function register() @@ -27,13 +38,11 @@ public function register() public function record(QueryExecuted $queryExecuted) { - $maximumQueries = $this->app['config']->get('flare.reporting.maximum_number_of_collected_queries', 200); + $this->queries[] = Query::fromQueryExecutedEvent($queryExecuted, $this->reportBindings); - $reportBindings = $this->app['config']->get('flare.reporting.report_query_bindings', true); - - $this->queries[] = Query::fromQueryExecutedEvent($queryExecuted, $reportBindings); - - $this->queries = array_slice($this->queries, $maximumQueries * -1, $maximumQueries); + if (is_int($this->maxQueries)) { + $this->queries = array_slice($this->queries, -$this->maxQueries); + } } public function getQueries(): array @@ -51,4 +60,28 @@ public function reset() { $this->queries = []; } + + public function getReportBindings(): bool + { + return $this->reportBindings; + } + + public function setReportBindings(bool $reportBindings): self + { + $this->reportBindings = $reportBindings; + + return $this; + } + + public function getMaxQueries(): ?int + { + return $this->maxQueries; + } + + public function setMaxQueries(?int $maxQueries): self + { + $this->maxQueries = $maxQueries; + + return $this; + } } diff --git a/tests/LogRecorder/LogRecorderTest.php b/tests/LogRecorder/LogRecorderTest.php new file mode 100644 index 00000000..cda731a1 --- /dev/null +++ b/tests/LogRecorder/LogRecorderTest.php @@ -0,0 +1,71 @@ +app, 200); + + foreach (range(1, 400) as $i) { + $log = new MessageLogged('info', 'test ' . $i, []); + $recorder->record($log); + } + + $this->assertCount(200, $recorder->getLogMessages()); + $this->assertSame('test 201', $recorder->getLogMessages()[0]['message']); + } + + /** @test */ + public function it_does_not_limit_the_amount_of_recorded_queries() + { + $recorder = new LogRecorder($this->app); + + foreach (range(1, 400) as $i) { + $log = new MessageLogged('info', 'test ' . $i, []); + $recorder->record($log); + } + + $this->assertCount(400, $recorder->getLogMessages()); + $this->assertSame('test 1', $recorder->getLogMessages()[0]['message']); + } + + /** @test */ + public function it_does_not_record_log_containing_an_exception() + { + $recorder = new LogRecorder($this->app); + + $log = new MessageLogged('info', 'test 1', ['exception' => new Exception('test')]); + $recorder->record($log); + $log = new MessageLogged('info', 'test 2', []); + $recorder->record($log); + + $this->assertCount(1, $recorder->getLogMessages()); + $this->assertSame('test 2', $recorder->getLogMessages()[0]['message']); + } + + /** @test */ + public function it_does_not_ignore_log_if_exception_key_does_not_contain_exception() + { + $recorder = new LogRecorder($this->app); + + $log = new MessageLogged('info', 'test 1', ['exception' => 'test']); + $recorder->record($log); + $log = new MessageLogged('info', 'test 2', []); + $recorder->record($log); + + $this->assertCount(2, $recorder->getLogMessages()); + $this->assertSame('test 1', $recorder->getLogMessages()[0]['message']); + $this->assertSame('test 2', $recorder->getLogMessages()[1]['message']); + $this->assertIsArray($recorder->getLogMessages()[0]['context']); + $this->assertArrayHasKey('exception', $recorder->getLogMessages()[0]['context']); + $this->assertSame('test', $recorder->getLogMessages()[0]['context']['exception']); + } +} diff --git a/tests/QueryRecorder/QueryRecorderTest.php b/tests/QueryRecorder/QueryRecorderTest.php index e27b3c4d..a8927655 100644 --- a/tests/QueryRecorder/QueryRecorderTest.php +++ b/tests/QueryRecorder/QueryRecorderTest.php @@ -12,7 +12,7 @@ class QueryRecorderTest extends TestCase /** @test */ public function it_limits_the_amount_of_recorded_queries() { - $recorder = new QueryRecorder($this->app); + $recorder = new QueryRecorder($this->app, true, 200); $connection = app(Connection::class); foreach (range(1, 400) as $i) { @@ -23,4 +23,49 @@ public function it_limits_the_amount_of_recorded_queries() $this->assertCount(200, $recorder->getQueries()); $this->assertSame('query 201', $recorder->getQueries()[0]['sql']); } + + /** @test */ + public function it_does_not_limit_the_amount_of_recorded_queries() + { + $recorder = new QueryRecorder($this->app, true); + $connection = app(Connection::class); + + foreach (range(1, 400) as $i) { + $query = new QueryExecuted('query '.$i, [], time(), $connection); + $recorder->record($query); + } + + $this->assertCount(400, $recorder->getQueries()); + $this->assertSame('query 1', $recorder->getQueries()[0]['sql']); + } + + /** @test */ + public function it_records_bindings() + { + $recorder = new QueryRecorder($this->app, true); + $connection = app(Connection::class); + + $query = new QueryExecuted('query 1', ['abc' => 123], time(), $connection); + $recorder->record($query); + + $this->assertCount(1, $recorder->getQueries()); + $this->assertSame('query 1', $recorder->getQueries()[0]['sql']); + $this->assertIsArray($recorder->getQueries()[0]['bindings']); + $this->assertSame('query 1', $recorder->getQueries()[0]['sql']); + $this->assertSame(123, $recorder->getQueries()[0]['bindings']['abc']); + } + + /** @test */ + public function it_does_not_record_bindings() + { + $recorder = new QueryRecorder($this->app, false); + $connection = app(Connection::class); + + $query = new QueryExecuted('query 1', ['abc' => 123], time(), $connection); + $recorder->record($query); + + $this->assertCount(1, $recorder->getQueries()); + $this->assertSame('query 1', $recorder->getQueries()[0]['sql']); + $this->assertNull($recorder->getQueries()[0]['bindings']); + } }