Opened 7 months ago

Closed 7 months ago

#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 7 months ago.
http lib unit test valgrind output

Download all attachments as: .zip

Change History (11)

comment:1 follow-up: Changed 7 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 7 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 7 months ago by marcin

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

comment:4 Changed 7 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 7 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:6 follow-up: Changed 7 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 7 months ago by tmark (previous) (diff)

Changed 7 months ago by tmark

http lib unit test valgrind output

comment:7 Changed 7 months ago by tmark

  • Owner changed from tmark to marcin

comment:8 in reply to: ↑ 6 Changed 7 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 7 months ago by tmark

  • Owner changed from tmark to marcin

The changes are fine, please merge.

comment:10 Changed 7 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.