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.

Sign up for GitHub

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

Jump to bottom

Relegate the on-close for emergencies #1175

Merged
merged 4 commits into from
Jul 5, 2022
Merged

Relegate the on-close for emergencies #1175

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b39c768
private/python_ipc_popen2: Fixes warning after unloading package.
Jul 4, 2022
0495049
Testing anonymous only onCleanup
cbm755 Jul 3, 2022
50799bb
Ensure pid is non-empty before waiting on it
cbm755 Jul 4, 2022
d300ea6
is_valid_file_id instead of not isempty
cbm755 Jul 5, 2022
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
31 changes: 27 additions & 4 deletions inst/private/python_ipc_popen2.m
View file
Open in desktop
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 @@ -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
cleanup = []; % note: triggers emergency file-close
fin = [];
fout = [];
pid = [];
A = true;
return
end

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

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

cleanup = onCleanup (@() ...
then (@(varagin) ...
[is_valid_file_id(fin) && fclose(fin), ...
is_valid_file_id(fout) && fclose(fout)], ...
@(varargin) waitpid(pid)));
Copy link
Collaborator Author

@cbm755 cbm755 Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

how big a crime is closing here without calling waitpid? Then we would not need the then...

Note: in the explicit reset case (and a glorious future where pkg unload has a hook), we will have waited on pid earlier and all of these variables will be empty.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we don't waitpid, we'll be left with a zombie process python3 <defunct>, because fclose (fin) will terminate the child process (i.e. python3). On the other hand, if we waitpid before fclose (fin), it will block, because waitpid will wait for the child process (i.e. python3) to terminate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks!

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
View file
Open in desktop

This file was deleted.