Skip to content

Conversation

@cnaples79
Copy link

Summary

  • Fix resource leak when reconfiguring logger multiple times
  • Prevent duplicate log messages from multiple subscriptions
  • Clean up existing file sinks before creating new ones

Changes Made

  • Added cleanup logic in configureLogger() function
  • Clear existing Logger.root listeners before adding new ones
  • Close existing fileSink and reset to null before creating new one
  • Prevents accumulation of listeners and file handles

Test Plan

  • Verified the fix addresses the root cause described in the issue
  • Code follows existing patterns used in terminate() function
  • Changes are minimal and focused on the specific problem

Fixes #6

Clean up existing logger subscriptions and file sink before
creating new ones in configureLogger to prevent duplicates
and resource leaks.

- Clear existing Logger.root listeners before adding new ones
- Close existing fileSink before creating new one
- Set fileSink to null for clean state

Fixes intel#6
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

cnaples79 and others added 3 commits September 29, 2025 19:36
- Add meta import and use unawaited() to fix discarded future warning
- Create comprehensive test for configureLogger reconfiguration
- Test verifies cleanup prevents duplicate logging and resource leaks
- Test covers edge cases including null fileSink handling
- Test ensures multiple reconfigurations work correctly

Addresses maintainer feedback on intel#6
mkorbel1
mkorbel1 previously approved these changes Sep 29, 2025
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you very much for the contribution!

@mkorbel1
Copy link
Contributor

@cnaples79 I noticed in the CI failure (https://github.com/intel/rohd-bridge/actions/runs/18113877408/job/51545958555) it only said one file needed reformatting:

Run tool/gh_actions/verify_formatting.sh
  tool/gh_actions/verify_formatting.sh
  shell: /usr/bin/bash -e {0}
  env:
    DART_HOME: /opt/hostedtoolcache/dart/3.9.3/x64
    PUB_CACHE: /home/ubuntu/.pub-cache
Changed test/rohd_bridge_logger_test.dart
Formatted 51 files (1 changed) in 1.16 seconds.
Format check failed: please format your code (use "dart format .")!

But your most recent commit seems to have reformatted many of the files. Perhaps you have some unusual configuration on the line width or changed a setting in analysis_options/pubspec.yaml that was not committed? Either way, ideally this PR doesn't need to reformat the entire repository.

@cnaples79
Copy link
Author

@mkorbel1 Ahh, its because I used dart format on the whole branch. I'll fix and update the PR.

Fix formatting issues in the newly added test file to pass CI checks.
Only format the test file that was added in this PR, not all files.
@cnaples79 cnaples79 force-pushed the fix-logger-subscription-cleanup branch from 9ce934c to 9163264 Compare September 30, 2025 22:42
@cnaples79
Copy link
Author

@mkorbel1 hopefully fixed now!

mkorbel1
mkorbel1 previously approved these changes Oct 1, 2025
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you for the contribution!!

@mkorbel1
Copy link
Contributor

mkorbel1 commented Oct 1, 2025

@cnaples79 looks like there's a handful of lint and import issues to clean up remaining

@cnaples79
Copy link
Author

@mkorbel1 hopefully this will do it! PR updated.

@cnaples79
Copy link
Author

@mkorbel1 hmm dart format on the logger file says no changes. Any idea why the CI is still failing?

@cnaples79
Copy link
Author

@mkorbel1 I re-ran dart format on the logger file and it updated this time. PR is updated. I had to upgrade my version of dart first, must have been using an older version.

@mkorbel1
Copy link
Contributor

mkorbel1 commented Oct 2, 2025

You can run the tool/run_checks.sh script locally to help catch issues before waiting for the CI to run. You seem to mostly be hitting issues with formatting and analysis.

@cnaples79
Copy link
Author

@mkorbel1 Thanks for the feedback. All tests from run_checks.sh are passing for me locally now.

Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting things passing, sorry I didn't notice this one before!

bool enableDebugMesage = false,
}) {
// Clean up existing logger subscriptions and file sink
Logger.root.clearListeners();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't notice this before, but I think we actually want to capture the StreamSubscription returned by Logger.root.onRecord.listen and cancel that, rather than clear all the listeners on Logger.root. It is possible that users would have been adding their own listeners to Logger.root for other purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean logger subscriptions upon reconfiguration

2 participants