py/persistentcode: always call reader->close() from mp_raw_code_load() - #3874

I decided to include a patch to resolve #3843 as well. Open to ideas on ways to improve this, possibly with less code?

Also kind of related is #3478.

Open to ideas on ways to improve this, possibly with less code?

One option is to refactor parsing and loading of .mpy files so they don't raise exceptions, but instead return error codes. But this is difficult because this code relies on some object constructors (eg makeing new strings, ints, floats, complex's) and all these constructors would need an alternative version that didn't raise an exception.

We are somehow going to need to add tests that test the invalid-mpy parts of the code.

We are somehow going to need to add tests that test the invalid-mpy parts of the code.

Added in 6d8816f

There's a well-known pattern of handling exceptions - at the "top level" (instead of many intermediate levels), and that's what MicroPython follows pretty much. For example, all calls mp_parse() are already wrapped in nlr_push(), and if something needs to be done about its lex param, that would be the place, instead of bloating up the stack of intermediate functions.

Also note that object deallocation in MicroPython is "best effort", not everything is freed/deallocated explicitly, because there's GC, which will take care of that. For handling non-memory resources, like open file descriptors, there's GC finalization support.

I think it's safer to wrap the function with an exception handler, as I've done, but I'm wondering about some other idiom for that code. For example, what if we replace this:

    nlr_buf_t nlr;
    if (nlr_push(&nlr) == 0) {
        mp_parse_tree_t rc = _mp_parse(lex, input_kind);
        nlr_pop();

        // free the lexer on behalf of the caller before returning
        mp_lexer_free(lex);
        return rc;
    } else {
        // exception; free lexer and re-raise same exception
        mp_lexer_free(lex);
        nlr_jump(nlr.ret_val);
    }

with this:

    mp_parse_tree_t rc;
    nlr_buf_t nlr;
    if (nlr_push(&nlr) == 0) {
        rc = _mp_parse(lex, input_kind);
        nlr_pop();
    }
    // free the lexer on behalf of the caller before returning
    mp_lexer_free(lex);

    if (failed) {
        // re-raise exception
        nlr_jump(nlr.ret_val);
    }
    return rc;

I'm not sure what to substitute for the failed conditional at the end. I like this format as it eliminates duplicated code (call to mp_lexer_free()) which makes maintenance easier and potentially reduces code size (mp_parse_compile_execute() has duplicated calls to mp_globals_set() and mp_locals_set()). Do you make failed a boolean initialized at the start of the function and set/cleared after the nlr_pop()? Can we look at something in the nlr_buf_t structure to know whether it was successful or not? Maybe initialize nlr.ret_val to NULL (as part of nlr_push() to reduce duplicated code) and check that?

And for reference, here's the test sequence I was using to validate my fix:

>>> import os
>>> with open("testmodule.mpy", "w") as testmodule:
...     testmodule.write("not valid")
... 
9
>>> import testmodule
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: incompatible .mpy file
>>> os.remove("testmodule.mpy")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 7001] EPERM 

And a version for the .py file just used '(((\r' for the file contents.

>>> import testmodule
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/flash/testmodule.py", line 2
SyntaxError: invalid syntax

@dpgeorge Any thoughts on merging this code? I've held off on it in our MicroPython fork but may need to merge it so we're not leaving files open on a failed import.

@dpgeorge We're planning to ship these changes in an upcoming firmware release. Wondering if it can get some consideration for upstream merge.

The Travis failure is just an (expected) increase in code size:

Old size: 71620 new size: 71656
Validation failure: Core code size increased

is just an (expected) increase in code size:

Well, not just the code size. As was explained above, this also increases the stack usage noticeably, and possibly recursively (explicit analysis of that should be persistent part of this change, i.e. be in the commit messages).

And I don't even mention coverage decrease (love "we can do it later" excuse myselef).

but I'm wondering about some other idiom for that code

Yes, there's another idiom: #4131

This is still a problem. Apart from the solution here (which uses a lot of stack), one way to tackle it is to make sure finalisers are used for all streams used by the parser/raw-code-loader, and then do a GC collect after importing (to make sure any non-freed streams are freed by the GC).

Alternatively, #4131 would be the best way to go.

I'll close this in favour of #12369, which uses much less C stack space to implement the same thing.

Also, the test here doesn't work on Linux (it passes even without the fix provided in this PR).