Opened 4 years ago

Closed 4 years ago

#4492 closed defect (fixed)

Kea servers need to explicitly unload hooks libs prior to exiting

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

Description (last modified by tmark)

Hook libraries are currently being unloaded as part of static object destruction during process shutdown. This means the order this occurs is indeterminate and can cause Hook unload() functions to be called well after things like the logger MessageInitializers? have been destroyed causing any log statements issued during the hook's unload() to fail with missing placeholders. It could also induce any number of other odd issues for hook developers.

We need to ensure the Hooks libraries are unloaded prior to exit() by invoking HooksManager::getHooksManager().unloadLibraries().

Per Stephen:

[09:30:44] <stephen> (A better way of doing it would be to create an object with nothing but a destructor that calls that function, then declare this new object in main(). That way, however the process exits, the object will be deleted and the libraries unloaded.)

See the attached patch (dhcp4 only)

Subtickets

Attachments (1)

preexit.patch (7.4 KB) - added by tmark 4 years ago.
Exit class

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by tmark

Exit class

comment:1 Changed 4 years ago by tmark

  • Description modified (diff)

comment:2 Changed 4 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:3 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 0 to 0.5
  • Estimated Difficulty changed from 0 to 1
  • Total Hours changed from 0 to 0.5

Rather than add a new class, I simply added the call to the destructors Dhcpv4Srv::~Dhcpv4Srv() and Dhcpv6Srv::~Dhcpv6Srv().

I'd suggest the following ChangeLog entry:

11xx.   [bug]       tmark
    An explicit call to unload the hook libraries prior to server
    exit was added to both kea-dhcp6 and kea-dhcp4.  This corrects
    an issue where loggging components were being destroyed prior
    to hook librarires being unloaded.
    (Trac #4492, git TBD)

comment:4 Changed 4 years ago by tmark

  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing

comment:5 Changed 4 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:6 Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

I reviewed commit 98141971d3f9657c6dfeed741fba9834613f27ee.

Your code changes look fine. Though, I think this change would require some unit tests. If nothing else, the test could simply create an instance of the DhcpvXSrv class, load a test library (e.g. src/lib/hooks/tests/.libs/), destroy the instance and verify that there are no callouts present?

comment:7 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 0.5 to 1
  • Owner changed from tmark to marcin
  • Total Hours changed from 0.5 to 1.5

I have added unit tests to verify unload upon server destruction. There were already test hook libraries for both servers so I reused these to formulate tests to specifically verify unload via destruction.

comment:8 follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit 05561a60d5569b999b97451c24323af5c60f5ebd

src/bin/dhcp4/tests/hooks_unittest.cc
src/bin/dhcp6/tests/hooks_unittest.cc

Thanks for implementing the tests. They are simple and elegant.

A couple nits.

The includes should be:

#include <dhcp4/tests/marker_file.h>
#include <dhcp4/tests/test_libraries.h>

rather than:

#include "marker_file.h"
#include "test_libraries.h"

Copyright should be updated.

It'd be useful to provide a comment before

IfaceMgr::instance().deleteAllExternalSockets();

to explain why this is needed for this particular set of tests.

Once you do it, you're ok to merge. I don't need to see it again.

comment:9 in reply to: ↑ 8 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 1 to 2
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.5 to 3.5

Replying to marcin:

Reviewed commit 05561a60d5569b999b97451c24323af5c60f5ebd

src/bin/dhcp4/tests/hooks_unittest.cc
src/bin/dhcp6/tests/hooks_unittest.cc

Thanks for implementing the tests. They are simple and elegant.

You're welcome and thanks. Not sure about the elegant part, I just made use of existing libraries that somebody else created.

A couple nits.

The includes should be:

#include <dhcp4/tests/marker_file.h>
#include <dhcp4/tests/test_libraries.h>

rather than:

#include "marker_file.h"
#include "test_libraries.h"

Oops and fixed.

Copyright should be updated.

Ugh. Fixed.

It'd be useful to provide a comment before

IfaceMgr::instance().deleteAllExternalSockets();

to explain why this is needed for this particular set of tests.

It isnt' needed so I removed it. It was left over from the class I stole.

Once you do it, you're ok to merge. I don't need to see it again.

Changes merged with git commit 2a4792b3551cce2fb9147f33f032ae7e71791d21
Added ChangeLog? entry 1112.

Ticket is complete.

Note: See TracTickets for help on using tickets.