Skip to content

Conversation

@amirnik
Copy link

@amirnik amirnik commented Sep 5, 2018

Issue #267:
Freeing first element in the Queue constructor

Issue #282:
Freeing allocated addrs structure after use.

@amirnik
Copy link
Author

amirnik commented Sep 6, 2018

Hello,
These are small changes which should be easy to review.

Would it be possible for you guys to kindly look at it and discuss / merge if needed?

Best Regards,
Amir

TRY(::listen(fd, backlog_));
break;
}
freeaddrinfo(addrs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works in the common case, but will still fail to clean up resources if the "::listen()" fails 2 lines above (the 'TRY' macro will throw a different exception).

I suggest that you encapsulate 'getaddrinfo' into a RAII class so that it will clean itself up no matter how the method exits.

@amirnik
Copy link
Author

amirnik commented Sep 10, 2018

For some reason build is not triggered on my commit...

@dennisjenkins75
Copy link
Collaborator

dennisjenkins75 commented Sep 11, 2018

gcc-7.3.0:

$ ./build/tests/run_http_client_test 
Running main() from gtest_main.cc
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from request_builder
[ RUN      ] request_builder.multiple_send_test
Got exception: Failed to connect: Address family not supported by protocol
/home/djenkins/src/oktal/pistache/tests/http_client_test.cc:56: Failure
Value of: response_counter == RESPONSE_SIZE
  Actual: false
Expected: true
Segmentation fault

Thread 1 "run_http_client" received signal SIGSEGV, Segmentation fault.
0x000055555570e4b6 in __gnu_cxx::__exchange_and_add(int volatile*, int) ()
(gdb) bt
#0  0x000055555570e4b6 in __gnu_cxx::__exchange_and_add(int volatile*, int) ()
#1  0x000055555570e54d in __gnu_cxx::__exchange_and_add_dispatch(int*, int) ()
#2  0x0000555555715059 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() ()
#3  0x0000555555712881 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() ()
#4  0x0000555555711236 in std::__shared_ptr<Pistache::Async::Private::Core, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() ()
#5  0x0000555555711278 in std::shared_ptr<Pistache::Async::Private::Core>::~shared_ptr() ()
#6  0x00005555557112ba in Pistache::Async::Resolver::~Resolver() ()
#7  0x00005555557bf762 in Pistache::Http::Transport::ConnectionEntry::~ConnectionEntry() ()
#8  0x00005555557c6b3e in Pistache::Queue<Pistache::Http::Transport::ConnectionEntry>::Entry::~Entry() ()
#9  0x00005555557c5728 in Pistache::Queue<Pistache::Http::Transport::ConnectionEntry>::~Queue() ()
#10 0x00005555557c2cf0 in Pistache::PollableQueue<Pistache::Http::Transport::ConnectionEntry>::~PollableQueue() ()
#11 0x00005555557caabe in Pistache::Http::Transport::~Transport() ()
#12 0x00005555557caaf6 in Pistache::Http::Transport::~Transport() ()
#13 0x00005555557d0fc8 in std::_Sp_counted_ptr<Pistache::Http::Transport*, (__gnu_cxx::_Lock_policy)2>::_M_dispose() ()
#14 0x000055555571507a in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() ()
#15 0x0000555555712881 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() ()
#16 0x00005555557bef9e in std::__shared_ptr<Pistache::Http::Transport, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() ()
#17 0x00005555557befba in std::shared_ptr<Pistache::Http::Transport>::~shared_ptr() ()
#18 0x00005555557b9a44 in Pistache::Http::Client::~Client() ()
#19 0x000055555570eb2b in request_builder_multiple_send_test_Test::TestBody() ()
#20 0x000055555574bad2 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#21 0x0000555555746a87 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#22 0x000055555572c0d4 in testing::Test::Run() ()
#23 0x000055555572c924 in testing::TestInfo::Run() ()
#24 0x000055555572cf9a in testing::TestCase::Run() ()
#25 0x0000555555733dcc in testing::internal::UnitTestImpl::RunAllTests() ()
#26 0x000055555574cc93 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ()
#27 0x000055555574792f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ()
#28 0x00005555557329b4 in testing::UnitTest::Run() ()
#29 0x0000555555757518 in RUN_ALL_TESTS() ()
#30 0x00005555557574a7 in main ()

@dennisjenkins75
Copy link
Collaborator

This PR has merge conflicts, and the impacted code has since been heavily refactored. I'm going to close this PR, but welcome any future PRs. Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants