#5260 closed defect (fixed)

Some Http tests still fail on Fedora 25 system after fixes with #5217.

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.2-final
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Subtickets

Attachments (1)

valgrind.out (12.0 KB) - added by tmark 15 months ago.
http lib unit test valgrind output

Download all attachments as: .zip

Change History (11)

comment:1 follow-up: Changed 15 months ago by fdupont

I have a Fedora 25 VM so I'll look at this (but at low priority as it is a Kea-proposed ticket...).

comment:2 in reply to: ↑ 1 Changed 15 months ago by marcin

Replying to fdupont:

I have a Fedora 25 VM so I'll look at this (but at low priority as it is a Kea-proposed ticket...).

I am working on this ticket and I almost have a fix ready. So, you don't need to do anything about it.

comment:3 Changed 15 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:4 Changed 15 months ago by marcin

  • Milestone changed from Kea-proposed to Kea1.2-final
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

This fix is now ready for review on the trac5260 branch. Proposed ChangeLog entry:

12XX.	[bug]		marcin
	Fixed failing unit tests in libkea-http.
	(Trac #5260, git abcd)

comment:5 Changed 15 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:6 follow-up: Changed 15 months ago by tmark

The changes look sane enough.

You should probably add some commentary inside HttpConnection::asyncAccept() and doWrite() that explains why passing in a local callback instance is safe: i.e underlying boost functions make handler copies as needed:

http://www.boost.org/doc/libs/1_60_0/doc/html/boost_asio/reference/basic_socket_acceptor/async_accept/overload1.html

When I first saw what you were doing it gave me pause, until I recalled having read something about handlers getting copied.

For grins, I ran http lib unit tests with valgrind:

==12860==
==12860== HEAP SUMMARY:
==12860==     in use at exit: 36,858 bytes in 546 blocks
==12860==   total heap usage: 25,234 allocs, 24,688 frees, 2,305,832 bytes allocated
==12860==
==12860== 12,153 (4,280 direct, 7,873 indirect) bytes in 1 blocks are definitely lost in loss record 527 of 529
==12860==    at 0x4C285FC: operator new(unsigned long) (vg_replace_malloc.c:298)
==12860==    by 0x4E98339: isc::http::HttpListenerImpl::accept() (listener.cc:172)
==12860==    by 0x4E9893D: isc::http::HttpListenerImpl::start() (listener.cc:151)
==12860==    by 0x429217: (anonymous namespace)::HttpListenerTest_requestTimeout_Test::TestBody() (listener_unittests.cc:439)
==12860==    by 0x4746E2: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2402)
==12860==    by 0x46748C: testing::Test::Run() (gtest.cc:2475)
==12860==    by 0x467523: testing::TestInfo::Run() (gtest.cc:2656)
==12860==    by 0x467624: testing::TestCase::Run() (gtest.cc:2774)
==12860==    by 0x4678AC: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4649)
==12860==    by 0x467B8D: testing::UnitTest::Run() (gtest.cc:2402)
==12860==    by 0x452C1D: main (gtest.h:2233)
==12860==
==12860== 12,325 (4,280 direct, 8,045 indirect) bytes in 1 blocks are definitely lost in loss record 528 of 529
==12860==    at 0x4C285FC: operator new(unsigned long) (vg_replace_malloc.c:298)
==12860==    by 0x4E98339: isc::http::HttpListenerImpl::accept() (listener.cc:172)
==12860==    by 0x4E9893D: isc::http::HttpListenerImpl::start() (listener.cc:151)
==12860==    by 0x428087: (anonymous namespace)::HttpListenerTest_listen_Test::TestBody() (listener_unittests.cc:339)
==12860==    by 0x4746E2: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2402)
==12860==    by 0x46748C: testing::Test::Run() (gtest.cc:2475)
==12860==    by 0x467523: testing::TestInfo::Run() (gtest.cc:2656)
==12860==    by 0x467624: testing::TestCase::Run() (gtest.cc:2774)
==12860==    by 0x4678AC: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4649)
==12860==    by 0x467B8D: testing::UnitTest::Run() (gtest.cc:2402)
==12860==    by 0x452C1D: main (gtest.h:2233)
==12860==
==12860== 12,380 (4,280 direct, 8,100 indirect) bytes in 1 blocks are definitely lost in loss record 529 of 529
==12860==    at 0x4C285FC: operator new(unsigned long) (vg_replace_malloc.c:298)
==12860==    by 0x4E98339: isc::http::HttpListenerImpl::accept() (listener.cc:172)
==12860==    by 0x4E9893D: isc::http::HttpListenerImpl::start() (listener.cc:151)
==12860==    by 0x428A87: (anonymous namespace)::HttpListenerTest_badRequest_Test::TestBody() (listener_unittests.cc:376)
==12860==    by 0x4746E2: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2402)
==12860==    by 0x46748C: testing::Test::Run() (gtest.cc:2475)
==12860==    by 0x467523: testing::TestInfo::Run() (gtest.cc:2656)
==12860==    by 0x467624: testing::TestCase::Run() (gtest.cc:2774)
==12860==    by 0x4678AC: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4649)
==12860==    by 0x467B8D: testing::UnitTest::Run() (gtest.cc:2402)
==12860==    by 0x452C1D: main (gtest.h:2233)
==12860==
==12860== LEAK SUMMARY:
==12860==    definitely lost: 12,840 bytes in 3 blocks

I've attached the whole output file for you.

Last edited 15 months ago by tmark (previous) (diff)

Changed 15 months ago by tmark

http lib unit test valgrind output

comment:7 Changed 15 months ago by tmark

  • Owner changed from tmark to marcin

comment:8 in reply to: ↑ 6 Changed 15 months ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

The changes look sane enough.

You should probably add some commentary inside HttpConnection::asyncAccept() and doWrite() that explains why passing in a local callback instance is safe: i.e underlying boost functions make handler copies as needed:

http://www.boost.org/doc/libs/1_60_0/doc/html/boost_asio/reference/basic_socket_acceptor/async_accept/overload1.html

When I first saw what you were doing it gave me pause, until I recalled having read something about handlers getting copied.

I have added commentaries as you say.

For grins, I ran http lib unit tests with valgrind:

==12860==
==12860== HEAP SUMMARY:
==12860==     in use at exit: 36,858 bytes in 546 blocks
==12860==   total heap usage: 25,234 allocs, 24,688 frees, 2,305,832 bytes allocated
==12860==
==12860== 12,153 (4,280 direct, 7,873 indirect) bytes in 1 blocks are definitely lost in loss record 527 of 529
==12860==    at 0x4C285FC: operator new(unsigned long) (vg_replace_malloc.c:298)
==12860==    by 0x4E98339: isc::http::HttpListenerImpl::accept() (listener.cc:172)
==12860==    by 0x4E9893D: isc::http::HttpListenerImpl::start() (listener.cc:151)
==12860==    by 0x429217: (anonymous namespace)::HttpListenerTest_requestTimeout_Test::TestBody() (listener_unittests.cc:439)
==12860==    by 0x4746E2: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2402)
==12860==    by 0x46748C: testing::Test::Run() (gtest.cc:2475)
==12860==    by 0x467523: testing::TestInfo::Run() (gtest.cc:2656)
==12860==    by 0x467624: testing::TestCase::Run() (gtest.cc:2774)
==12860==    by 0x4678AC: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4649)
==12860==    by 0x467B8D: testing::UnitTest::Run() (gtest.cc:2402)
==12860==    by 0x452C1D: main (gtest.h:2233)
==12860==
==12860== 12,325 (4,280 direct, 8,045 indirect) bytes in 1 blocks are definitely lost in loss record 528 of 529
==12860==    at 0x4C285FC: operator new(unsigned long) (vg_replace_malloc.c:298)
==12860==    by 0x4E98339: isc::http::HttpListenerImpl::accept() (listener.cc:172)
==12860==    by 0x4E9893D: isc::http::HttpListenerImpl::start() (listener.cc:151)
==12860==    by 0x428087: (anonymous namespace)::HttpListenerTest_listen_Test::TestBody() (listener_unittests.cc:339)
==12860==    by 0x4746E2: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2402)
==12860==    by 0x46748C: testing::Test::Run() (gtest.cc:2475)
==12860==    by 0x467523: testing::TestInfo::Run() (gtest.cc:2656)
==12860==    by 0x467624: testing::TestCase::Run() (gtest.cc:2774)
==12860==    by 0x4678AC: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4649)
==12860==    by 0x467B8D: testing::UnitTest::Run() (gtest.cc:2402)
==12860==    by 0x452C1D: main (gtest.h:2233)
==12860==
==12860== 12,380 (4,280 direct, 8,100 indirect) bytes in 1 blocks are definitely lost in loss record 529 of 529
==12860==    at 0x4C285FC: operator new(unsigned long) (vg_replace_malloc.c:298)
==12860==    by 0x4E98339: isc::http::HttpListenerImpl::accept() (listener.cc:172)
==12860==    by 0x4E9893D: isc::http::HttpListenerImpl::start() (listener.cc:151)
==12860==    by 0x428A87: (anonymous namespace)::HttpListenerTest_badRequest_Test::TestBody() (listener_unittests.cc:376)
==12860==    by 0x4746E2: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2402)
==12860==    by 0x46748C: testing::Test::Run() (gtest.cc:2475)
==12860==    by 0x467523: testing::TestInfo::Run() (gtest.cc:2656)
==12860==    by 0x467624: testing::TestCase::Run() (gtest.cc:2774)
==12860==    by 0x4678AC: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4649)
==12860==    by 0x467B8D: testing::UnitTest::Run() (gtest.cc:2402)
==12860==    by 0x452C1D: main (gtest.h:2233)
==12860==
==12860== LEAK SUMMARY:
==12860==    definitely lost: 12,840 bytes in 3 blocks

I've attached the whole output file for you.

I created #5261 to address that.

comment:9 Changed 15 months ago by tmark

  • Owner changed from tmark to marcin

The changes are fine, please merge.

comment:10 Changed 15 months ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged with commit 43394f76efb1634155c04b205dec7361fc21f4f9

Note: See TracTickets for help on using tickets.