Always close files (ReadDigraphs, WriteDigraphs)

[pramothragavan]

Related to #377, which ensured files close on error when decoding.

ReadDigraphs is leaving files open if a malformed file is fed in when using a whole file decoder (dreadnaut/dimacs) -- my bad. I had also carelessly left a file open in the tests. The suggested changes fixes those issues.

Something that's not my fault is that this issue propagates to encoding with WriteDigraphs but affects all file formats. I think this issue has probably been around for a while. If we encounter an error while encoding (i.e. due to the digraph being incompatible somehow with the file format), then the file isn't closed. For instance,

gap> filename := "/Users/pramothragavan/Downloads/10.txt";;
gap> D := Digraph([[2], []]);
<immutable digraph with 2 vertices, 1 edge>
gap> IO.OpenFiles;
[  ]
gap> WriteDigraphs(filename, D, Graph6String);
Error, the argument <D> must be a symmetric digraph with no loops or multiple edges, at /Users/pramothragavan/gap/pkg/Digraphs/gap/io.gi:2227 called from
encoder( D ) at /Users/pramothragavan/gap/pkg/Digraphs/gap/io.gi:246 called from
encoder( file, digraphs[i] ); at /Users/pramothragavan/gap/pkg/Digraphs/gap/io.gi:640 called from
<function "WriteDigraphs">( <arguments> )
 called from read-eval loop at *stdin*:8
type 'quit;' to quit to outer loop
brk> quit;
gap> IO.OpenFiles;
[ <file fd=4 wbufsize=65536 wdata=0> ]

I'm not really sure how to fix this, because you inevitably need to keep the file open while the digraph is being encoded. Is there maybe a good way of doing a try/catch style statement?

I've added a few lines to the tests to make sure the list of open files is the same before and after the test runs. Consequently, CI should fail here.

[pramothragavan]

Okay I think this works. The fix for ReadDigraphs is fairly trivial. For the bigger issue with WriteDigraphs, I've done something a bit hacky, but I'm not sure how else to go about it.

Basically we're suppressing errors (to stop premature break loops before the file is closed), but still 'catching' them. If we catch an error, we then close the file, restore normal errors, and then try to encode again (which is expected to give an error). i.e. we pass through the encoder twice if we know for sure that it will ErrorNoReturn. I think it's necessary to pass through the encoder the second time, rather than just erroring out immediately, so that the break loop manifests in the correct place.

This is obviously quite finicky. Some notes to make the review easier:

• if encoder is not a whole file encoder, then it is 'wrapped' in a way that calling encoder(file, D) encapsulates both encoding in the appropriate format and writing to the file. This 'wrapping' isn't a thing for whole file encoders, so SafeEncode also returns the encoded string, which then can be used for explicitly writing out.
• I don't see how anything other than the encoding of the digraph could cause an error here, so hopefully it's (just about) okay to do this. If we really wanted to be on the safe side, we could start/stop suppressing errors within SafeEncode (directly above and below line 639) so that we're dead certain the encoder is the only thing that is realising the effects of suppressed errors. But this would mean we're changing the error suppression variables (BreakOnError and SilentNonInteractiveErrors) more than needed, I reckon.

Hopefully the lines added to the tests will help avoid this problem in the future.

[james-d-mitchell]

Thanks @pramothragavan, it looks good, but I'll try to read this in detail today.

[fingolfin]

A potentially simpler solution which does not require encoding twice might be to do something like this on the start of your function:

    local ... oldOnBreak ...;
    ...
    oldOnBreak := OnBreak;
    OnBreakWithCleanup := function()  # giving this a name is not strictly necessary but helps when debugging
      CloseAndCleanup();
      OnBreak := oldOnBreak;
      OnBreak();
    end;
    OnBreak := OnBreakWithCleanup;

    ...

    # just before leaving the function, restore OnBreak
    OnBreak := oldOnBreak;
    return;
end;

[pramothragavan]

Very nice! A minor point -- if the user enters the break loop manually, the file would close and they'd be unable to reenter the main loop? Maybe this scenario's too unrealistic and not worth the trouble

[james-d-mitchell]

Good point @pramothragavan, I think we can live with that

[pramothragavan]

I've implemented these changes, but in test environments we don't enter the break loop, so I've had to manually restore OnBreak and then close the last file that was opened (assuming that IO.OpenFiles is a chronological list?). This doesn't feel particularly robust, let me know if there's a better way of handling tests.

Outside of the scope of tests, this works great -- thanks Max!

[james-d-mitchell]

Seems good, okay to merge @pramothragavan ?

[pramothragavan]

Yeah reckon so @james-d-mitchell, as long as we're happy with these caveats