Should it be necessary to `clear all` before unloading a pkg?

[cbm755]

I'm playing with:

podman exec oc octave --eval "pkg load symbolic; sympref diagnose; syms x; pkg unload symbolic; pkg list"

....

Your kit looks good for running the Symbolic package.  Happy hacking!

Symbolic pkg v2.9.0+: Python communication link active, SymPy v1.5.1.
Package Name  | Version | Installation directory
--------------+---------+-----------------------
    symbolic  |  2.9.0+ | /usr/share/octave/packages/symbolic-2.9.0+
warning: onCleanup: error caught while executing cleanup function:
'python_ipc_popen2_reset' undefined near line 78, column 30
error: ignoring const execution_exception& while preparing to exit

I cannot reproduce locally. I can fix it with pkg load symbolic; sympref diagnose; syms x; clear all; pkg unload symbolic; That is adding clear all. Which makes sense. What is supposed to happen?

---

Observed on: gnuoctave/octave:7.1.0 container Cannot reproduce: locally Fedora 35, Octave 6.4.0 from rpm.

[alexvong243f]

Do you mean docker exec oc... ?

[alexvong243f]

I can reproduce this error locally. In fact, the minimal reproducible example for me is octave --no-gui --eval "pkg load symbolic; sym('x'); pkg unload symbolic" with error message

Symbolic pkg v2.9.0+: Python communication link active, SymPy v1.10.1.
warning: onCleanup: error caught while executing cleanup function:
'python_ipc_popen2_reset' undefined near line 78, column 30

For reference, octave --no-gui --version outputs

GNU Octave, version 7.1.0
Copyright (C) 1993-2022 The Octave Project Developers.
This is free software; see the source code for copying conditions.
There is ABSOLUTELY NO WARRANTY; not even for MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE.

Octave was configured for "x86_64-pc-linux-gnu".

Additional information about Octave is available at https://www.octave.org.

Please contribute if you find this software useful.
For more information, visit https://www.octave.org/get-involved.html

Read https://www.octave.org/bugs.html to learn how to submit bug reports.

[alexvong243f]

Could the underlying issue be python_ipc_popen2_reset being a private function?

Running find octsympy/ -name '*reset*' returns

octsympy/inst/private/python_ipc_popen2_reset.m

[alexvong243f]

Running grep -r onCleanup returns

inst/private/python_ipc_popen2.m:    cleanup = onCleanup (@() python_ipc_popen2_reset (fin, fout, pid));

So the reason that clear all gets rid of the problem is because it clears all cleanup functions.

[cbm755]

Makes sense. I guess I wonder what is correct here? Like if you have objects created from a package and then you unload that package, it seems to me you are asking for trouble...

[alexvong243f]

Is it possible to customize what pkg unload do?

My intuition is that pkg unload symbolic should call and unregister cleanup functions created by the symbolic package before actually unloading (i.e. removing the variable bindings of the symbolic package from the global environment / symbol table).

More generally, it appears to me that sensible implementation of (pkg load, pkg unload) should satisfy the following properties:

1. loading twice is the same as loading once (idempotency)
2. unloading after loading should be equivalent to doing nothing (left inverse)
3. unloading if not loaded should be an error

But of course, we may not be able to achieve this if it is not possible to customize what pkg unload do...

[cbm755]

I don't recall there being a 'onUnload' hook, but I don't know. Take a look through the official package docs and then post a thread on discourse?

The current onCleanup hook is supposed to stop us from losing the two file descriptors (of Popen2).

[alexvong243f]

Both the documentation

     'unload'
          Remove named packages from the path.  After unloading a
          package it is no longer possible to use the functions provided
          by the package.  Trying to unload a package that other loaded
          packages still depend on will result in an error; no packages
          will be unloaded in this case.  A package can be forcibly
          removed with the '-nodeps' flag, but be aware that the
          functionality of dependent packages will likely be affected.
          As when loading packages, reloading dependencies after having
          unloaded them with the '-nodeps' flag may not restore all
          functionality of the dependent packages as the required
          loading order may be incorrect.

and the source code https://hg.savannah.gnu.org/hgweb/octave/file/04120d65778a/scripts/pkg/private/unload_packages.m don't indicate such functionality, I am pretty confident it doesn't exist.

If all the current onCleanup hook does is to free 2 fds, we can re-write the hook as an anonymous function without referencing any symbolic-specific functions (using some tricks). This arrangement won't satisfy the properties in my last comment, but maybe this is good enough for us (?)

[cbm755]

I see what you mean. I just looked at ...reset and it also tries to shutdown the pipe, has verbose settings etc. As you say "using some tricks" it should be possible to have a pure anonymous function here (but implies more maintenance burden I suspect?)

Seems to me this is a least somewhat an upstream issue?

For reference: https://octave.sourceforge.io/octave/function/onCleanup.html

How urgent do you think this is? I think onCleanup was added by @mtmiller during the time since the last release. If we release 3.0.0 "as-is" then anyone using Symbolic and explicitly unloading Symbolic will tend to get this error Warning.

I just noticed its a warning not an error so I think this is "paper cut" rather than bug... (?)

[cbm755]

So "needs decision" for question: "can we defer worrying about this?" If people are anything like me, they never "unload" packages in real life. And for the record, the MWE that users could reasonably expect is:

pkg load symbolic; x=sym('x'); x=42; pkg unload symbolic; clear all

That is, without that x=42, you still have sym variables around: unloading the package is asking for trouble and you get to keep both pieces. (This is unrelated to this bug as its not sym objects that have the onCleanup but rather a persistent variable inside a private ipc function).

[alexvong243f]

I can't recall the last time I use pkg unload. Having extra packages loaded have not caused issues for me in the past, so I don't find a need to unload them.

Regarding "using some tricks", what I mean is that we can define conditional as an anonymous function and for other things such as sequential execution / unordered execution as well. The end result is that we can do almost everything w/o using any statement. For instance, the following example cleanup function runs correctly w/o any warning:

    % see https://stackoverflow.com/questions/35115039
    if__ = @(varargin) varargin{3 - varargin{1}}();
    if_ = @(pred, if_clause, else_clause) if__(pred, if_clause, else_clause);
    % this will be called when we `clear cleanup`: avoids dangling file handles
    cleanup = onCleanup (@() ...
                          if_(randi(2) == 1, ...
                              @() fprintf('pid = %d, fin = %d\n', pid, fin), ...
                              @() fprintf('pid = %d, fout = %d\n', pid, fout)));

I don't think this will cause more maintenance burden because we don't have to change it once it's written. But we need comments to explain what's going on.

[cbm755]

wow! Shall we release as-is and see what happens?

If you take your approach, maybe we can move (well "move back") all the flush and wait logic to python_ipc_popen2.m, and have the onCleanup be a simpler "emergency close":

cleanup = onCleanup (@() [fclose(fin) fclose(fout)]);

(EXCEPT that will be an error if they are already closed)

[cbm755]

Oh, how about? is_valid_file_id(fin) && fclose(fin) using short-circuiting instead of if?

[alexvong243f]

Yes, simplifying the cleanup function sounds like a good idea to me!

[alexvong243f]

Oh, how about? is_valid_file_id(fin) && fclose(fin) using short-circuiting instead of if?

~I think is_valid_file_id is an Octave-only function.~ Well, we use Octave-only function popen2 in this part of code anyway, so it doesn't matter...