Skip to content

fix: Prevent race condition with better logging setup#109

Closed
matellush wants to merge 10 commits into
TeamWheelWizard:devfrom
matellush:fix/logger-setup
Closed

fix: Prevent race condition with better logging setup#109
matellush wants to merge 10 commits into
TeamWheelWizard:devfrom
matellush:fix/logger-setup

Conversation

@matellush
Copy link
Copy Markdown
Collaborator

@matellush matellush commented Apr 5, 2025

Purpose of this PR:

Fix the bug where settings are not loaded correctly because of the order of setup operations.

How to Test:

Run the program and see if everything is correctly logged (also to the log file). Also, test multiple times to check that the situation does not occur again. Additionally, test the case of #101 again with a corrupt JSON config file and see if it gets logged correctly.

What Has Been Changed:

The logger usage has been changed to use the new Log everywhere (also in GUI contexts), dropping the logging stuff from the GUI service provider while still having the Logger.Sink for Avalonia.

Related PR Links:

#108, #101

Checklist before merging

  • You have created relevant tests

RubenBroere
RubenBroere previously approved these changes Apr 5, 2025
Copy link
Copy Markdown
Member

@DirkDoes DirkDoes left a comment

Choose a reason for hiding this comment

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

it does not fix the problem that was being fixed in #108

Comment thread WheelWizard/Program.cs Outdated
@matellush
Copy link
Copy Markdown
Collaborator Author

My general suggestion would be to drop the logging part entirely from the services added to the service provider (also the adapter).

@matellush
Copy link
Copy Markdown
Collaborator Author

I dropped the stupid additional service provider stuff I had in there and used an ILoggerFactory instead, with the logger cleanup called from Main directly. This makes more sense to me than having logger services in the GUI service provider in the first place.

@matellush
Copy link
Copy Markdown
Collaborator Author

it does not fix the problem that was being fixed in #108

Even then, this is a better approach to logging than having to use the service provider in our code to access the logger.

@patchzyy
Copy link
Copy Markdown
Member

patchzyy commented Apr 6, 2025

I get where you are coming from, but the use of static app.services has already led to some weird race conditions (because then we are dependent on the order of the operations in the static state of the program), that kind of global state makes it harder to reason about what is initialized when and where. We dont want to introduce tight coupling between parts of the app.

By moving this logger into DI we have more predictable behaviour.
at the end of the day we DONT want a static service provider, which IS what we have now, but keeping it the way we have it now, when we get rid of that it needs to be removed anyways. So then adding all this extra code that will be deleted anyways is unneeded

@patchzyy patchzyy closed this Apr 6, 2025
@matellush
Copy link
Copy Markdown
Collaborator Author

I get where you are coming from, but the use of static app.services has already led to some weird race conditions (because then we are dependent on the order of the operations in the static state of the program), that kind of global state makes it harder to reason about what is initialized when and where. We dont want to introduce tight coupling between parts of the app.

By moving this logger into DI we have more predictable behaviour.
at the end of the day we DONT want a static service provider, which IS what we have now, but keeping it the way we have it now, when we get rid of that it needs to be removed anyways. So then adding all this extra code that will be deleted anyways is unneeded

Oh, so you would rather want different logging approaches between setup and GUI code and increase unnecessary coupling to 1st Serilog and 2nd ServiceProviders. It doesn't even make sense to go out of one's way to log through the service provider, that's extremely ugly and at the end of the day, we would want to incentivize logging, not make it obscure! It's completely valid to initialize logging at the very beginning and then use the static logger across the entire code and then dispose the logger at the end (one could also make a finally block to clean up the logger in my version for completeness' sake).

The argument that you're going to rewrite it anyway doesn't really make sense to me since that's still going to take a lot of work, and one could use a uniform logging approach in the meantime. The other PR that was merged also assigns a static logger, after all, which is not better at all.

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.

4 participants