Skip to content

Conversation

@zsmb13
Copy link
Collaborator

@zsmb13 zsmb13 commented Nov 5, 2025

Breaks the dependency cycle between the storage class and the logger so that the storage implementation can also contribute to logs.

Fixes #544

Breaks the dependency cycle between the storage class and the logger so that the storage implementation can also contribute to logs.
@zsmb13 zsmb13 marked this pull request as ready for review November 5, 2025 17:11
@zsmb13 zsmb13 requested a review from kropp November 5, 2025 17:11
Comment on lines 50 to 54
buffer.forEach { entry ->
realLogger.log(entry.tag) { entry.message }
}
buffer.clear()
delegate = realLogger
Copy link
Member

Choose a reason for hiding this comment

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

We may lose some messages here, both between forEach and clear, and between clear and setting delegate.
If adding a lock is undesirable, I'd suggest at least draining the buffer again after setting the delegate.
Another improvement would be to remember the last dumped message, cleaning the buffer only until that point, and again re-dumping the rest after a delegate is assigned.

Copy link
Collaborator Author

@zsmb13 zsmb13 Nov 6, 2025

Choose a reason for hiding this comment

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

I wanted to avoid a lock on the happy path of logging into a delegate that's already set.

But I now added locking for working with the buffer now. Any of those interactions should only happen during app launch and never past that.

@kropp
Copy link
Member

kropp commented Nov 7, 2025

I cannot get rid of the feeling, that we can resolve settings <-> logging dependency in some other way, without overcomplicating logging.

@zsmb13
Copy link
Collaborator Author

zsmb13 commented Nov 7, 2025

@kropp I'd love to have a cleaner solution as well, but I can't think of anything obviously better.

Some alternatives that I see would be:

  • Reading just the debugLogging flag from storage by direct access and with no logging, going through a different interface than MultiplatformSettingsStorage, before we run storage-related migrations or anything like that.
  • Have a proxy like we have now with BufferedDelegatingLogger, with a different implementation: we can make it create a new coroutine for each log call which will each suspend while the delegate is unavailable, and then continue and log once the delegate is set.
  • Skip logging until we read the flags from storage and know what to do about logging - but this would lose important logs like the ones about storage migrations when we want to debug that.

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.

Add logging to SettingsStorage

3 participants