Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relegate the on-close for emergencies #1175

Merged
merged 4 commits into from
Jul 5, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions inst/private/python_ipc_popen2.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
%% Copyright (C) 2014-2019, 2022 Colin B. Macdonald
%% Copyright (C) 2018, 2020 Mike Miller
%% Copyright (C) 2022 Alex Vong
%%
%% This file is part of OctSymPy.
%%
Expand Down Expand Up @@ -34,7 +35,7 @@

function [A, info] = python_ipc_popen2(what, cmd, varargin)

persistent cleanup fin fout pid
persistent emergency_cleanup fin fout pid

py_startup_timeout = 30; % seconds

Expand All @@ -43,11 +44,29 @@
info = [];

if (strcmp(what, 'reset'))
cleanup = []; % triggers a call to reset function
if (~ isempty (pid))
if (verbose)
disp ('Closing the Python communications link.')
end
end
A = true;
if (~ isempty (fin))
% produces a single newline char: not sure why
t = fclose (fin);
fin = [];
waitpid (pid);
pid = [];
A = A && (t == 0);
end
if (~ isempty (fout))
alexvong243f marked this conversation as resolved.
Show resolved Hide resolved
t = fclose (fout);
fout = [];
A = A && (t == 0);
end
emergency_cleanup = []; % note: triggers emergency file-close
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this line. Why do we need to "trigger emergency file-close"? IIUC we've just cleaned things up above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its going to get triggered eventually. Might as well trigger it now, when I know it is an no-op.

I guess I'm vague uncomfortable with this split: here on line 63 we close fout, let's say its 24. My understanding of how anonymous functions work is that the onCleanup still has "24" hardcoded in it (it has no idea fout has been replaces with []).

Some other fopen call later reuses 24.

Now our ticking timebomb onCleanup goes off and closes that 24. I don't know if that can really happen but the idea that it might means I don't like that onCleanup lying around. Let it go off right now while its harmless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I should have a comment that gives that idea: % trigger while we know it is a no-op?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this should make things clearer

fin = [];
fout = [];
pid = [];
A = true;
return
end

Expand All @@ -74,9 +93,13 @@
error('popen2() failed');
end

% this will be called when we `clear cleanup`: avoids dangling file handles
cleanup = onCleanup (@() python_ipc_popen2_reset (fin, fout, pid));

then = @(f, g) g (f ());
% will be called when we `clear functions`: avoids dangling file handles
emergency_cleanup = onCleanup (@() ...
then (@(varagin) ...
[is_valid_file_id(fin) && fclose(fin), ...
is_valid_file_id(fout) && fclose(fout)], ...
@(varargin) ~isempty(pid) && waitpid(pid)));
headers = python_header();
fputs (fin, headers);
fprintf (fin, '\n\n');
Expand Down
53 changes: 0 additions & 53 deletions inst/private/python_ipc_popen2_reset.m

This file was deleted.