[RESOLVED] Q: Correct way to write to the log.

[genericeventhandler]

TL;DR: Exceptions are bad, what is the correct way to write to the log?

I'm currently looking at other peoples code to learn more about how things work, 

In the mod I'm looking at, no names, the developer is using Exceptions to log information.

I've been doing C# since before day 1, and Exceptions shouldn't be used to define logic, they are hugely memory consuming and slow. 

So what is the correct way to write to the log in KSP? I might change this mod as it's one I use regularly.

Thanks
GE

 

 

Edited July 25, 2016 by genericeventhandler
Answered

[Thomas P.]

UnityEngine.Debug.Log("Hello!")

might be what you are looking for

[genericeventhandler]

2 minutes ago, Thomas P. said:

UnityEngine.Debug.Log("Hello!")

might be what you are looking for

It's not the debug log, but the actual log file that he is trying to write to,  or is that the same thing?

[Thomas P.]

Debug.Log writes the given message to output_log.txt / Player.log / KSP.log. Thrown exceptions end up there too, unless you catch them and redirect them manually.

[genericeventhandler]

1 minute ago, Thomas P. said:

Debug.Log writes the given message to output_log.txt / Player.log / KSP.log. Thrown exceptions end up there too, unless you catch them and redirect them manually.

Cool thanks

[sarbian]

2 hours ago, genericeventhandler said:

In the mod I'm looking at, no names, the developer is using Exceptions to log information.

What do you mean by that ? The code uses throw to log ?

[genericeventhandler]

1 minute ago, sarbian said:

What do you mean by that ? The code uses throw to log ?

Psudeo code, because I don't want to point fingers.

If(vesselPart == null)
{
	throw new Exception("Vessel part not found");
}

and in the calling method 

try{
.....
} catch(Exception)
{
// log the message.  <<<< GE: psuedo comment in the code.
throw;
}

 

There are other instances, where it's just being used for logic

try{
   PerformActionOnPart();
}
catch (Exception)
{
// Part not found
return;
}

Probably a Java developer at heart, as exceptions are pretty lightweight in Java. No so in C#

GE

[sarbian]

6 minutes ago, genericeventhandler said:

Probably a Java developer at heart, as exceptions are pretty lightweight in Java. No so in C#

lightweight (and I would not agree here) does not make them a good way to handle logic.

 

The others cases have their use. The null check makes sense if it should never append and is not bad code IMHO. The catch and throw make handling some specific case easier to read but I agree that's not the kind of thing to abuse (I do it in MM where handling the exception deep in a coroutine would block the loading.)

[genericeventhandler]

I didn't say it was good programming practice, just overused in java. 

• In C# if you don't know how to handle something throw the correct exception, or a new exception inherited from System.Exception (never ApplicationException and never inherited from another exception)
• if the method you are calling throws exceptions, can you avoid those exceptions by doing some checking before?
• if you can't avoid the exception, let it bubble to a method that can handle it.
• Log if you catch the exception, but don't log if you throw; 

GE

 

[Diazo]

Unfortunately, there are several quirks present relative to exceptions that I'm not sure whether to blame on KSP or Unity that muddy this situation.

I don't think I am guilty of exactly anything you are posting here, but I do use some logic that is awfully close for reasons as follows:

1) KSP uses singletons quite a bit so I have not found any other way to handle something like this:

public class Master
{
	public static Master ourMaster
	public string ourText
}

//If ourMaster is not initialized yet...
public void Update()
{
	if(ourMaster != null) //returns true
	{
		if(ourMaster.ourText != null) //this throws a NullRef error instead of returning false, need a try/catch block to handle it
	}
}

2) KSP uses sequential routines a lot, for example the load routine:

public class ourPartModule : PartModule
{
	public ConfigNode Load(ConfigNode node) 
	{
		//stick any code here that throws a nullref error
	}
}

This nullref error causes the entire load routine to abort. Ran into this several months ago where only the root part would spawn as the load routine starts there, ksp would spawn the part and while trying to load the partModules on it run into this nullref error and KSP just threw its hands up and stopped the entire load routine leaving you with a single part visible in the editor.

Again, the only way I've ever found to handle this is a try/catch block so you keep the error contained within your own code, but I use the existing exceptions for that so I don't think it's exactly the same issue.

D.

Edited July 25, 2016 by Diazo

[genericeventhandler]

When dealing with statics, you need to lock the resource while you use it, I've seen a lot of bad singleton implementations in many addons. There is only ONE way to implement a singleton that is safe < .Net 4.0,  above .net 4.0 there is another way but It hides why it is thread safe so I don't advise using it. 

 public class SingletonClass
        {
            // this gets instanced once per singleton class created!
            private static readonly object LockObject = new object();

            // this gets set once per singleton class created!
            private static readonly SingletonClass instance;

            // hide the public creation of the class
            private SingletonClass()
            {
            }

            /// <summary>
            /// Gets an instance of the singleton class
            /// </summary>
            public SingletonClass Instance
            {
                get
                {
                    // lock on the lock object, so only one LockObject is created, only one thread can pass through here
                    lock (LockObject)
                    {
                        // is the instance null?
                        if (instance == null)
                        {
                            // create a new singleton class
                            instance = new SingletonClass();
                        }

                        // return the new instance.
                        return instance;
                    }
                }
            }
        }

If someone tries to tell you that you should use an outer if around the lock, they are wrong. The optimiser will optimise away the internal if in that case.

If you use the pattern above you won't get null reference exceptions because multiple threads have created multiple versions of your singleton

here's why you get NRE's with your code. 

Your code. 

public class Master
{
	public static Master ourMaster
	public string ourText
}

//If ourMaster is not initialized yet...
public void Update()
{
	if(ourMaster != null) //returns true
	{
		if(ourMaster.ourText != null) //this throws a NullRef error instead of returning false, need a try/catch block to handle it
	}
}

Imagine 2 threads,   T1 is a few ms infront of T2. 

T1 : set ourMaster = new MasterT1

T1 : call update

T1: if(ourMaster != null)   -> true

T2: set ourMaster = new MasterT2    // previous MasterT1 falls out of scope

T1: if( ourMaster.ourText != null)   ----->>   ourMaster = MasterT1 = null -----> Null reference exception

           

With the locked instance, T2 cannot replace the Master object created by T1.

if you really don't want the locks you can create a new static Property like this, 

public static SingletonClass InstanceNotSafe
            {
                get
                {
                    if(instance == null)
                    {
                        return Instance;
                    }

                    return instance;
                }
            }

All threads will block on the instance, the first time around, but after it's been created it's semi safe to use.

 

 

 

[sarbian]

Nothing in the execution of a Unity Monobehaviour/GameObject is threaded.

[genericeventhandler]

4 minutes ago, sarbian said:

Nothing in the execution of a Unity Monobehaviour/GameObject is threaded.

It uses events though doesn't it?  anything that could delete your statics / reset them to a new object needs to be protected. 

 

[sarbian]

Nothing will interrupt your code. Using lock is going overboard there.

[Diazo]

@genericeventhandler The issue that I'm using that code to reference singeltons in the stock KSP code which I can't make changes to so I work around this issue with the try/catch block.

It works and I know of no better solution, and I've bounced this question off the forum and #KSPModders before (although months, maybe over a year, ago) and no one else had any ideas either.

D.