PaulBrack / Yamato

SWATH-QC metrics
Apache License 2.0
1 stars 1 forks source link

Unhandled Exception if a "File not found" is not caught, dumps core #79

Closed mwalzer closed 4 years ago

mwalzer commented 4 years ago
Yamato.Console -in napedro_L120420_010_SW.mzML
2019-12-04 10:34:32.4850|INFO|Yamato.Console.Program|Verbose output selected: enabled logging for all levels
2019-12-04 10:34:32.5146|INFO|Yamato.Console.Program|Loading file: n
2019-12-04 10:34:32.5221|ERROR|Yamato.Console.Program|Unable to open the file: n.

Unhandled Exception: System.IO.FileNotFoundException: Could not find file '/hps/nobackup2/proteomics/SwathMSdata/PASS00289/SGS/raw/n'.
   at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory, Func`2 errorRewriter)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String path, OpenFlags flags, Int32 mode)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode)
   at Yamato.Console.Program.CheckFileIsReadableOrComplain(String inputFilePath) in C:\Sandbox\github\Yamato\Console\Program.cs:line 103
   at Yamato.Console.Program.<>c.<Main>b__1_0(Options options) in C:\Sandbox\github\Yamato\Console\Program.cs:line 70
   at CommandLine.ParserResultExtensions.WithParsed[T](ParserResult`1 result, Action`1 action)
   at Yamato.Console.Program.Main(String[] args) in C:\Sandbox\github\Yamato\Console\Program.cs:line 22
Aborted (core dumped)

You'll quickly spot the 'intentional' error in the exec: it is -in instead of -i so it looks for the file n. Anyway, opening the inexistant file is attempted, not found, unhandled exception, core dump.

PaulBrack commented 4 years ago

This is expected behaviour - we catch the error, log it, then re-throw the error after catching to preserve the stack trace.

There are a million and one ways to handle errors but we decided to let the program fail noisily and drop me out of the executing process than catch errors rather than have to navigate a code path out of the process. Example below:

    private static void CheckFileIsReadableOrComplain(string inputFilePath)
    {
        try
        {
            Stream stream = new FileStream(inputFilePath, FileMode.Open);
            stream.Close();
        }
        catch (IOException)
        {
            logger.Error(String.Format("Unable to open the file: {0}.", inputFilePath));
            throw;
        }
    }

Is the core dump a problem we can work around?

Ozzard commented 4 years ago

I'm with Mathias on this one: the Console application should not dump core. It should do the most verbose logging possible of the exception and exit - essentially with: main() { try { rest of program } catch (Exception ex) { log(ex); return 1; } return 0; }

On a good day, one then puts #if !DEBUG...#endif markers around the tries and catches so that the exceptions still propagate up to the debugger when testing and debugging.

mwalzer commented 4 years ago

bump on v1.0.0-b

PaulBrack commented 4 years ago

Implemented fix as per Peter's suggestion