Skip to content

Conversation

@pjh
Copy link
Contributor

@pjh pjh commented Dec 2, 2023

To reproduce the memory leak cherry-pick this PR, ensure you have valgrind installed and then:

bazel build //ortools/base:hello_file
valgrind --leak-check=full ./bazel-bin/ortools/base/hello_file &> leak.out
bazel build --cxxopt=-DFIX_MEMORY_LEAK //ortools/base:hello_file
valgrind --leak-check=full ./bazel-bin/ortools/base/hello_file &> no-leak.out

Here's the output I see: leak.out, no-leak.out

valgrind reports "definitely lost: 48 bytes in 2 blocks" for or-tools at head, but "definitely lost: 0 bytes in 0 blocks" when the File is deleted after closing it.

bazel build //ortools/base:hello_file
./bazel-bin/ortools/base/hello_file
valgrind --leak-check=full ./bazel-bin/ortools/base/hello_file &> leak.out
bazel build --cxxopt=-DFIX_MEMORY_LEAK //ortools/base:hello_file
./bazel-bin/ortools/base/hello_file
valgrind --leak-check=full ./bazel-bin/ortools/base/hello_file &> no-leak.out
@pjh pjh force-pushed the demonstrate-memory-leak branch from 0318013 to df579a7 Compare December 2, 2023 01:06
@pjh pjh changed the title Demonstrate file::Open memory leak Demonstrate file::GetContents / SetContents memory leak Dec 2, 2023
@flomnes
Copy link
Contributor

flomnes commented Mar 22, 2024

Since this issue has been solved by

I think it's safe to close this ticket.

Personal opinion: returning std::unique_ptr<File> would be safer ie there would be no room for developers to forget resource liberation.

@pjh pjh closed this Mar 22, 2024
@Mizux Mizux self-assigned this Mar 25, 2024
@Mizux Mizux added this to the v10.0 milestone Mar 25, 2024
@Mizux Mizux added Bug Lang: C++ Native implementation issue labels Mar 25, 2024
@Mizux Mizux modified the milestones: v10.0, v9.10 Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Lang: C++ Native implementation issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants