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

fix: File descriptor leaks for eventfd and timerfd_create #1158

Merged
merged 2 commits into from
Nov 6, 2023
Merged

fix: File descriptor leaks for eventfd and timerfd_create #1158

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4ec2fb
fix(server): file descriptor leaks for eventfd and timerfd_create
tyler92 Aug 4, 2023
e6f27c2
fix(server): separate library for descriptors leaks detection
tyler92 Nov 5, 2023
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
1 change: 1 addition & 0 deletions include/pistache/os.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ namespace Pistache
{
public:
NotifyFd();
~NotifyFd();

Polling::Tag bind(Polling::Epoll& poller);

Expand Down
9 changes: 9 additions & 0 deletions src/common/os.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,15 @@ namespace Pistache
: event_fd(-1)
{ }

NotifyFd::~NotifyFd()
{
if (event_fd >= 0)
{
close(event_fd);
event_fd = -1;
}
}

Polling::Tag NotifyFd::bind(Polling::Epoll& poller)
{
event_fd = TRY_RET(eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC));
Expand Down
11 changes: 11 additions & 0 deletions src/server/endpoint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace Pistache::Http
using Base = Tcp::Transport;

explicit TransportImpl(const std::shared_ptr<Tcp::Handler>& handler);
~TransportImpl() override;

void registerPoller(Polling::Epoll& poller) override;
void onReady(const Aio::FdSet& fds) override;
Expand All @@ -53,8 +54,18 @@ namespace Pistache::Http
TransportImpl::TransportImpl(const std::shared_ptr<Tcp::Handler>& handler)
: Tcp::Transport(handler)
, handler_(handler)
, timerFd(-1)
{ }

TransportImpl::~TransportImpl()
{
if (timerFd >= 0)
{
close(timerFd);
timerFd = -1;
}
}

void TransportImpl::registerPoller(Polling::Epoll& poller)
{
Base::registerPoller(poller);
Expand Down
45 changes: 45 additions & 0 deletions tests/http_server_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@
#include <string>
#include <thread>

#if __has_include(<filesystem>)
#include <filesystem>
namespace filesystem = std::filesystem;
#else
#include <experimental/filesystem>
namespace filesystem = std::experimental::filesystem;
#endif

#include "tcp_client.h"

using namespace Pistache;
Expand Down Expand Up @@ -1043,3 +1051,40 @@ TEST(http_server_test, server_with_content_encoding_deflate)
ASSERT_EQ(originalUncompressedData, newlyDecompressedData);
}
#endif

TEST(http_server_test, http_server_is_not_leaked)
{
const auto number_of_fds = [] {
using filesystem::directory_iterator;
const filesystem::path fds_dir { "/proc/self/fd" };

if (!filesystem::exists(fds_dir))
{
return directory_iterator::difference_type(0);
}

return std::distance(directory_iterator(fds_dir), directory_iterator {});
};

const auto fds_before = number_of_fds();
const Pistache::Address address("localhost", Pistache::Port(0));

auto server = std::make_unique<Http::Endpoint>(address);
auto flags = Tcp::Options::ReuseAddr;
auto server_opts = Http::Endpoint::options().flags(flags).threads(4);
server->init(server_opts);
server->setHandler(Http::make_handler<PingHandler>());
server->serveThreaded();
server->shutdown();
server.reset();

if (fds_before > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Under what conditions would you expect number_of_fds() to return 0? gunit's main() does not close STDOUT nor STDERR, so there should always be at least two.
  2. Why test for this condition AFTER running the HTTP server? Why not test for it on line 1069?
  3. Do you think that it is important to process an actual HTTP request within this test?
    4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The answer is simple - only if /proc/self/fd directory doesn't exist. I know that the library supports only Linux now, but I wasn't sure if I could make this assumption for unit tests. Do you recommend removing this check?
  2. There is no significant reason, just to make the unit test less noisy
  3. For this particular leak - it doesn't matter. Regarding leaks in general, I think it's important to test a realistic use case. So if you recommend adding it - I will

{
const auto fds_after = number_of_fds();
ASSERT_EQ(fds_before, fds_after);
}
else
{
std::cout << "NOTE: Please use Valgrind with '--track-fds=yes' option for this test" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

@tyler92, I am a bit concerned about this. Does the user need to run the unit test in valgrind(1) in order to verify that it passed? The reason I ask is it would be cumbersome for a user to have to manually do that, as opposed to the build environment automatically doing that. If it can't do that, then if the test fails we'll never know during CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. It's related to the first question from another thread. The logic here is the following - we need to handle the case when our "leak detector" fails. In this case, we can do one of the following: fail the test, pass the test, or pass with a warning. I chose the last option, but if we have full confidence in this leak detection approach, we may fail the test.

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't have full confidence yet in this approach and it might need refactoring. You might need to modify the build environment itself to ensure that the test is run via valgrind(1). You'd then need to add the valgrind package to debian/control in the Build-Dep stanza as well as making sure the build environment finds it at configure time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kiplingw Probably there is a misunderstanding. When I said "leak detection approach", I meant not valgrind, but checking file descriptors count. Now I suggest removing anything about Valgrind in my test, and it will look like this:

TEST(http_server_test, http_server_is_not_leaked)
{
    const auto number_of_fds = [] {
        using filesystem::directory_iterator;
        const filesystem::path fds_dir { "/proc/self/fd" };

        if (!filesystem::exists(fds_dir))
        {
            return directory_iterator::difference_type(0);
        }

        return std::distance(directory_iterator(fds_dir), directory_iterator {});
    };

    const auto fds_before = number_of_fds();
    ASSERT_GT(fds_before, 0);

    // ...

    const auto fds_after = number_of_fds();
    ASSERT_EQ(fds_before, fds_after);

Valgrind was a fallback option in my original idea, but it has downsides.

Copy link
Member

Choose a reason for hiding this comment

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

@dennisjenkins75, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Since you probably intend to remove the Valgrind fallback this isn't very relevant, but in general when skipping tests in GoogleTest you should use GTEST_SKIP() (documented at https://google.github.io/googletest/advanced.html#skipping-test-execution) since it will properly signal to the testing environment that a test case was skipped.

}
}
2 changes: 1 addition & 1 deletion version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.2.3.20231022
0.2.4.20231031
Loading