Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate body/asteroid loader fix #302

Merged
merged 2 commits into from
Aug 1, 2018

Conversation

Corfiot
Copy link
Contributor

@Corfiot Corfiot commented Jul 31, 2018

Duplicate bodies/asteroids cause duplicate loggers to be created, throwing IOExceptions and killing loading. Now loader skips duplicated entries gracefully.

Duplicate bodies/roids cause duplicate loggers to be created, throwing IOExceptions and killing loading. Now loader skips duplicated entries gracefully.
}
catch (Exception e)
{
bodyLogger.LogException(e);
Logger.Default.Log("[Kopernicus]: Configuration.Loader: Failed to load Body: " + bodyNode.GetValue("name"));
//bodyLogger.LogException(e); -- This would be nice but becomes a problem when duplicate loggers are created: can't log then
Copy link
Member

Choose a reason for hiding this comment

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

Since you already check for a duplicated body before the try / catch block starts, this could stay as it is right now. The logger shouldn't throw because of duplicated names anymore, so it doesn't need to be created in a try / catch.

@Corfiot
Copy link
Contributor Author

Corfiot commented Aug 1, 2018

The whole loading process seems to be aborted when a single log file fails to be created. This is why I covered it.

@StollD
Copy link
Member

StollD commented Aug 1, 2018

Then please declare the bodyLogger variable outside of the try / catch but assign it inside. That way, the catch block can do a null-check on it and still log errors that are related to loading the body in the body log.

@Corfiot
Copy link
Contributor Author

Corfiot commented Aug 1, 2018

The problem happened when the object was constructed not when it was set as active logger. Maybe we can create a simple constructor for it, then we can do as you suggest safely.

@StollD
Copy link
Member

StollD commented Aug 1, 2018

Or we just do this

Logger bodyLogger;
try
{
    bodyLogger = new Logger("..");
    ...
}
catch (Exception e)
{
    if (bodyLogger != null) 
    {
        bodyLogger.LogException(e);
    }
    else
    {
        Logger.Default.LogException(e);
    }
    ...
}

@Corfiot
Copy link
Contributor Author

Corfiot commented Aug 1, 2018

I'm not that good at C# yet, does a constructor that throw an exception set the instance to null?
Give me some time, I'll propose a new set that is similar and has the advantage or using a single Logger instance for all bodies, preventing possible garbage collection issues. I think that incorporates your strategy.

@StollD
Copy link
Member

StollD commented Aug 1, 2018

I'm not that good at C# yet, does a constructor that throw an exception set the instance to null?

Yes, thats what happens. The class cannot be initialized so the variable retains the previous value (which is null by default)

Logger class can now be instantiated without a filename and switch filenames. Loader now uses a single Logger instance.
@Corfiot
Copy link
Contributor Author

Corfiot commented Aug 1, 2018

Works exactly as before with less garbage collection, no null-checks and a smarter logger.

@StollD
Copy link
Member

StollD commented Aug 1, 2018

Looks good, thank you!

Merging

@StollD StollD merged commit 58d72c2 into Kopernicus:master Aug 1, 2018
@Corfiot Corfiot deleted the duplicate-body-loader-fix branch August 1, 2018 21:15
@Corfiot
Copy link
Contributor Author

Corfiot commented Aug 1, 2018

Thanks for the feedback.

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.

None yet

2 participants