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

Fix FILE* resource leak in agread() wrapper #415

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

nehaljwani
Copy link
Contributor

@nehaljwani nehaljwani commented Jun 6, 2022

Resolves #213

@nehaljwani nehaljwani marked this pull request as draft June 6, 2022 04:22
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2022

Codecov Report

Merging #415 (98802cc) into main (3053c66) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #415   +/-   ##
=======================================
  Coverage   80.87%   80.87%           
=======================================
  Files           5        5           
  Lines        1255     1255           
=======================================
  Hits         1015     1015           
  Misses        240      240           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3053c66...98802cc. Read the comment docs.

@nehaljwani nehaljwani changed the title Add test to probe repeated IO on Windows. Fix FILE* resource leak in agread() wrapper Jun 6, 2022
@nehaljwani nehaljwani marked this pull request as ready for review June 6, 2022 05:06
@jarrodmillman jarrodmillman added this to the 1.10 milestone Jun 10, 2022
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Thanks @nehaljwani !

@rossbar
Copy link
Contributor

rossbar commented Aug 10, 2022

Ack, I just noticed that the changes here modify the generated graphviz_wrap.c file directly... this means that the changes would be overridden/lost if/when the C code is regenerated w/ SWIG.

In other words, the changes to graphviz_wrap.c should be instead moved to graphviz.i. Unfortunately the mapping from SWIG definition to C-code is not immediately clear to me... @Nagael , any chance there is a quick fix here? IYO is there a straightforward way to modify graphviz.i to result in the changes we see in graphviz_wrap.c here in this diff?

@dschult
Copy link
Contributor

dschult commented Aug 10, 2022

I'm not sure, and I don't have a setup to test this, but it looks like the following might work:

line 18: add the dup() around fd

        $1 = fdopen(dup(fd), mode);

line 94-96: add a line to fclose(arg1);

%exception agread {
  $action
  fclose(arg1);               // added line
  if (!result) {

@rossbar
Copy link
Contributor

rossbar commented Aug 10, 2022

Yeah that was my first thought as well... unfortunately the first line is in a templated function, so it affects two wrapped functions - one of which then causes an infinite hang when running the test suite.

@Nagael
Copy link
Contributor

Nagael commented Aug 11, 2022

Hi,
I have a proposition, which I described in #432, and you can see it in this commit . It passes all tests on my setup, but I have no Windows system to test.
Good luck !

rossbar added a commit to rossbar/pygraphviz that referenced this pull request Oct 3, 2023
* Add test to probe repeated IO on Windows.

* Fix FILE* resource leak in agread() wrapper

Resolves pygraphviz#213

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Pygraphviz crashes after drawing 170 graphs on Windows
6 participants