Throw error instead of exiting silently

[DanielHabenicht]

EnoEngine/EnoEngine/Program.cs

Lines 34 to 38 in 950a3da

	if (!mutex.WaitOne(10, false))
	{
	Console.WriteLine("Another Instance is already running.");
	return;
	}

is just one of example of silently exiting.

It should throw errors instead:

throw new Exception("Config (ctf.json) does not exist.");

[Trolldemorted]

Well it is not silent, it prints a very clear message why it exits early. Last year students complained when they saw stacktraces, so we switched to messages only :P

Should we switch to stderr and return a non-zero exit code?

[Savallator]

Yeah, Stacktraces do not really help there... also keep in mind that the engine is not inside docker. The Console.Writelines plop directly into your shell, it is really hard to miss.

[DanielHabenicht]

What? Why did they complain about stack traces? That's normal in IT. 😂
I mean if they don't learn it here, where else?

It is displaying the same message just with more information and in Visual Studio it even opens a popup that something went wrong instead of just exiting silently.

I get it that for newcomers it may make things more complicated, but programming is based on standards and these should be adhered to.

[Trolldemorted]

What? Why did they complain about stack traces? That's normal in IT. 😂

To be fair, that's normal for crappy programs, but not the general state of IT. Imagine linux utils like cat printing stack traces if they can't open a file, that would be a horrible UX. Instead, they print a concise errormessage cat: ./foo: No such file or directory and exit.

[ldruschk]

But these tools do exit with a non-zero exit code

[Trolldemorted]

But these tools do exit with a non-zero exit code

Changing that should be easy, just return 1 I think

[DanielHabenicht]

We could also wrap the program into an ExceptionHandler as you already do it in the lines below.

This way throughout the program you could still use Exceptions and then write a nice output with the Release build. (while keeping the stack traces during development)
Something along:

catch (Exception e)
{
#if (DEBUG)
    throw e;
#else
    Console.WriteLine(e.Message);
    return 1;
#endif
}
finally
{
    mutex?.Close();
}
return 0;

[Savallator]

But you will never actually need a Stacktrace in this particular case. It is just stupid to make it return a useless stacktrace instead of a single message to exactly tell you what the problem is. As Trolldemorted said, you won't want cat to print a stacktrace as well when the file not exists. And bloating the code with useless #IF wont do any good.

[DanielHabenicht]

I agree that you don't need an actual Stacktrace in the case I provided and also for running it in release.
But for development, it makes things easier.

Just to name an example where it actually hurts:

EnoEngine/EnoEngine/Program.cs

Lines 66 to 70 in 950a3da

	catch (JsonConfigurationValidationException e)
	{
	Console.WriteLine($"Configuration is invalid: {e.Message}");
	return;
	}

Here the child Exception is swallowed and will never be displayed. If the service for a checker is not reachable the only message displayed will be Configuration is invalid., whereas with proper Exception:

In order to not mix up both, I added the general case in the upper comment.

[Savallator]

I don't really get what you are trying to do. The code you referenced belongs to the checking of the config file. The stacktrace there does not contain any useful information, and the exception message is already printed. The HTTPRequestException you posted is something else entirely. The case when a checker is not reachable should also not throw an Exception, but should exit with the appropriate Error or continue for testing, as discussed in #144. What would an exception with a stacktrace into the HTTPClient actually help anyone, when it is as simple as a connection being refused?

[DanielHabenicht]

This issue is about the general practice of not throwing errors but just silently exiting (during development).
As I said there no useful message is provided because it just prints to console, furthermore I am not even getting the whole error because it just prints Configuration is invalid. although the issue is with the service not reachable. A Stacktrace would have helped resolve this issue faster.

I tried to run the Engine but kept on running into issues.
This is something that would have helped me fix the mistakes I did during startup much faster. (e.g. not running a checker before starting the engine)

Until #144 is implemented this is still an issue and will be in the future for any other nested Exception.

What would an exception with a stacktrace into the HTTPClient actually help anyone, when it is as simple as a connection being refused?

Instead of just seeing a Configuration is invalid actually knowing where the error is. Currently, this is pointing in a totally different direction.

[Savallator]

The issue is not the service being unreachable, not even the checker being unreachable. The issue is exactly what is printed - the Config is invalid.
If the checker is actually not reachable, it prints exactly that: Service checker failed to respond to info request (service 1).
I just now tested it.