bugGNU Octave - Bugs: bug #54180, libinterp/corefcn/file-io.cc-tst...

 
 

bug #54180: libinterp/corefcn/file-io.cc-tst failure and weird output when test suite is run twice

Submitted by:  Andrew Janke <apjanke>
Submitted on:  Sun 24 Jun 2018 10:51:00 PM UTC  
 
Category:  Test Suite Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  4.4.0 Operating System:  Mac OS

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 29 Jun 2018 09:10:57 PM UTC, comment #20:

This is fine. Originally, I thought about using whos because there was extra information--specifically the global flag--which I thought might provide a way to ensure that no variables changed nature during the tests. But the way you did it seems like it will be faster and have a smaller memory footprint.

If you create a global variable in the base workspace this is reported twice: 1) as a variable created in the base workspace, and 2) as a global variable created in the base workspace. Could eliminate that by using setdiff one more time to subtract the global variables from the local variables. But, it probably isn't worth it. This shouldn't be happening very frequently and should be corrected immediately.

Rik <rik5>
Project Administrator
Fri 29 Jun 2018 08:35:26 PM UTC, comment #19:

I pushed a change that checks for global variables:

http://hg.savannah.gnu.org/hgweb/octave/rev/48a9bd2d0a20

I also changed the check for variables in the base workspace to just use "who" rather than "whos" and to only check whether the output of setdiff (new, old) is empty. Rik, is there something that I'm not seeing that requires whos and size_equal?

Is there anything else we should be checking?

John W. Eaton <jwe>
Project Administrator
Fri 29 Jun 2018 06:26:22 PM UTC, comment #18:

I added a check that now new variables are created in the base workspace (http://hg.savannah.gnu.org/hgweb/octave/rev/90c8325c14be). I benchmarked it briefly and it didn't seem to contribute much, if anything, to running time so I think its fine to have this check in all the time.

Rik <rik5>
Project Administrator
Fri 29 Jun 2018 05:24:07 PM UTC, comment #17:

If it is as short as the file-descriptor check, we could add the variable checks to the development branch. If issues arise, they'll be recognized by developers in a short time and the changes could be backed out easy enough...or modify the test script to generate the required variables with each individual test.

Dan Sebald <sebald>
Fri 29 Jun 2018 04:49:27 PM UTC, comment #16:

For the development branch, I pushed a cset (http://hg.savannah.gnu.org/hgweb/octave/rev/a87b4dca2ce3) that will emit a warning if a file has %!tests which leak file descriptors.

Do we want to do something similar for global variables? How about for any variables? I would think that the ideal tests are run in their own workspace and don't pollute the base workspace at all. Normally, that would only mean checking for new globals, but since there are tests for evalin it is possible that local variables could also be created in the caller workspace which would be the base workspace.

Rik <rik5>
Project Administrator
Wed 27 Jun 2018 04:39:21 AM UTC, comment #15:

Great. Marking as fixed and closing report.

Rik <rik5>
Project Administrator
Wed 27 Jun 2018 02:36:11 AM UTC, comment #14:

Looks fixed!

I built from Mercurial, and the error went away: I ran _run_test_suite_ repeatedly in the GUI, and libinterp/corefcn/file-io.cc-tst passed each time.

Andrew Janke <apjanke>
Tue 26 Jun 2018 04:24:25 PM UTC, comment #13:

I committed Dan's cset here http://hg.savannah.gnu.org/hgweb/octave/rev/6aa29920a531.

@Andrew: Can you download the latest stable code from Mercurial and test whether the problem is cured? Marking as "Ready for Test".

Rik <rik5>
Project Administrator
Mon 25 Jun 2018 07:32:15 PM UTC, comment #12:

Here's a new changeset that adds various unwind_protect blocks. Let me know if it as least on the target somewhere. In some cases the fclose() doesn't have any commands prior to it that will error, so those don't need to be protected. There are a few others near the end for which the fclose could possibly moved earlier and avoid the need for an unwind_protect--in fact, those might use the same "nm = tempname (); ... unlink(nm)" approach to not leave the small temporary files hanging about. But I do leave the temporary file around when there is an error in case someone needs to debug by looking at that file contents.

(file #44435)

Dan Sebald <sebald>
Mon 25 Jun 2018 05:13:05 PM UTC, comment #11:

Concur with jwe. If the patch is updated to use unwind/protect blocks it can be applied immediately to the stable branch.

On the dev branch, the test m-file could be augmented to use

Rik <rik5>
Project Administrator
Mon 25 Jun 2018 12:05:06 PM UTC, comment #10:

Dan, your change is OK, but should probably use an unwind_protct block to also avoid leaks in case there is an error in the test. I think that's what we use in other tests.

Checking for leaked resources in the function that runs tests for a given file seems like a good change. Other than the things mentioned in comment #9, another thing to check for might be global variables.

I don't think you can check for leaked resources in a single %!test though, because multiple tests may share the same open file, global variable, or other resource.

Probably it should just be an error if there is a leak since it is an error in the test suite. If that turns out to cause too much trouble, then we could make it something that is not fatal and is just reported in the summary like other test failures.

John W. Eaton <jwe>
Project Administrator
Mon 25 Jun 2018 09:38:32 AM UTC, comment #9:

I think this patch worked!

I rebuilt my Octave against it and ran _run_test_suite_ multiple times in a session; the `file-io.cc-test` failure did not recur.

I did get other failures during my reruns:

From fntests.log:

Those look related to graphics support, not file handles. And maybe they're because I was doing other things with my machine while the tests ran.

> Could you make that just one change within the test() function?


I will make a patch to do this if I find some time.

Are there other global resources we should check for leakage of? Figure handles? Open network ports?

Do you think it's appropriate to call error() to abort the tests in the case of leakage? Or introduce a new "ERROR: ..." output to report them? Or something else?

Andrew Janke <apjanke>
Mon 25 Jun 2018 07:40:43 AM UTC, comment #8:

Here's a changeset. (Yes, it does look like the lines you mentioned.) This seems to pass the test.

Good idea about the leaked file descriptors. Seems like a good thing to have. Could you make that just one change within the test() function? That seems the logical place to have the test.

(file #44433)

Dan Sebald <sebald>
Mon 25 Jun 2018 07:29:48 AM UTC, comment #7:

Hey, look at io.tst lines 514 through 537.

That `fopen (nm, "rb")` inside the loop looks like a dangling fopen() to me.

And then in lines 576 through 611:

That `id = fopen (nm, "rb");` inside the loop at line 589 doesn't seem to have a corresponding fclose().

And that would account for the two leaked file descriptors.

Andrew Janke <apjanke>
Mon 25 Jun 2018 07:13:25 AM UTC, comment #6:

Nice approach!

I modified your code to directly detect leaked file descriptors in the test. See attachment HACK-detect-leaked-file-descriptors-in-tests.patch.

Using this detection method, I declare `io.tst` to be the culprit.

Andrew Janke <apjanke>
Mon 25 Jun 2018 06:23:30 AM UTC, comment #5:

In the last post I didn't rewrite what I had intended as I was watching the original result. The sentence

"Throughout the whole test suite there is no file-io.cc-tst fail on the first pass. On the second pass, it starts failing immediately:"

is incorrect and should have been rewritten something like:

The tests make it all the way to "Fixed test script" test/io.tst before file-io-.cc-tst starts failing. See the following output (in comment #4) where I've snipped out many tests that passed successfully.

Dan Sebald <sebald>
Mon 25 Jun 2018 06:18:14 AM UTC, comment #4:

I made a little mod to the scripts/testfun/__run_test_suite__.m (attached) which will run file-io.cc-tst after every test. I wanted to see exactly at what point file-io.cc-tst starts failing.

Throughout the whole test suite there is no file-io.cc-tst fail on the first pass. On the second pass, it starts failing immediately:

I believe there are one or two missing "fclose()" calls in the io.tst. I've made some changes and started rerunning the test. If it works I'll post a changeset.

(file #44430)

Dan Sebald <sebald>
Mon 25 Jun 2018 04:38:38 AM UTC, comment #3:

Thanks; that helps narrows things down.

Running file-io.cc-tst on its own repeated without error is a clue... I've managed to isolate and repeat the third case you cited. The slightly more verbose result is

So this example is expecting textscan (single (40)) to fail, but it isn't failing. That suggests to me that some test after file-io.cc-tst is opening a bunch of file, all the way up to file ID "40", without closing that file (probably many files). Since file-io.cc-tst on its own is not failing, we need to find that later test which is opening files but not closing them.

Dan Sebald <sebald>
Mon 25 Jun 2018 01:01:32 AM UTC, comment #2:

> exit the GUI, re-launch the GUI, run test
> exit the GUI, re-launch the GUI, run test
> exit the GUI, re-launch the GUI, run test


Did this. The failure did not appear. Sounds like there might be a GUI-session-scoped cleanup happening, as you suspect.

Another weird thing: Running just `file-io.cc-tst` on its own repeatedly in the GUI does not cause this failure:

Andrew Janke <apjanke>
Sun 24 Jun 2018 11:56:12 PM UTC, comment #1:

Could you try

exit the GUI, re-launch the GUI, run test
exit the GUI, re-launch the GUI, run test
exit the GUI, re-launch the GUI, run test

and see if the failure appears. Perhaps something files aren't being properly closed at the end of the test and another test has problems opening the file. Exiting the GUI might close those files. Just a guess.

The other odd thing is what looks like an incomplete summary of the error report. The test indicates 1 failed test, and I suspect the data between curly brackets is the result, but there should be some columns indicating what test failed, what was expected and what was observed. I wonder if Octave has those items going to a different I/O stream, whereas in the command line version both end up going to the shell terminal.

Dan Sebald <sebald>
Sun 24 Jun 2018 10:51:00 PM UTC, original submission:

This is in Octave 4.4.0 on macOS 10.13.5, built with the Qt GUI.

When I run `__run_test_suite__` twice in a row inside the Qt GUI, the `libinterp/corefcn/file-io.cc-tst` produces some weird output and fails one test on the second and later runs.

From the second run:

From the third and later runs:

This is only when running the test suite in the GUI; it doesn't seem to happen when run in command line mode.

Full test results (from the fourth run in a session) attached.

Andrew Janke <apjanke>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by apjanke (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-06-27 rik5 StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2018-06-26 rik5 StatusIn Progress => Ready For Test
    2018-06-25 sebald Attached File- => Added octave-fclose_missing_from_io_test-djs2018jun25.patch, #44435
    2018-06-25 rik5 StatusNone => In Progress
    2018-06-25 sebald Attached File- => Added octave-fclose_missing_from_io_test-djs2018jun24.patch, #44433
    2018-06-25 apjanke Attached File- => Added HACK-detect-leaked-file-descriptors-in-tests.patch, #44432
    2018-06-25 sebald Attached File- => Added run_file-io_test_after_every_test-djs2018jun24.diff, #44430

    Back to the top


    Powered by Savane 3.3