Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4060 closed defect (fixed)

Kea is now multi-threaded

Reported by: fdupont Owned by: fdupont
Priority: high Milestone: Kea1.0-beta
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

Description

After the timer manager introduction Kea changes from mono-threaded to multi-threaded. Some configs should be adjusted accordingly, e.g.:

  • no {BOOST_}ASIO_DISABLE_THREADS (requires #4009)
  • initialize Botan in thread_safe mode
  • same for OpenSSL

Note if log4cplus already handles this I don't know for mysql or postgresql...

Low priority as no problem is expected.

Subtickets

Change History (14)

comment:1 Changed 4 years ago by fdupont

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

comment:2 Changed 4 years ago by fdupont

Should also block all signals in not main threads (so signals are delivered only to the main thread). (Mandatory, I create a specific ticket for it).
process_spawn and signal_set have to be rewritten:

  • no longer use sigprocmask
  • can be called only by the main thread

comment:3 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from low to high

per team meeting 23 sept, move to 1.0 high

comment:4 Changed 4 years ago by fdupont

Cleanup the thread related stuff in configure.ac and Makefile.am files.

comment:5 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Rebased branch trac4060a is ready for review. It doesn't include the cryptolink thread safe code (I'll put it in a specific branch if wanted).
Note I kept the BOOST_ASIO_DISABLE_THREADS flag even there is no good reason (perhaps to remove it will solve more problems than it created). It is a good candidate for a new configuration flag but IMHO we have already a lot and no need for more complexity.

comment:6 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits 6dbe6ff310ecd230425c6ebfe18f72d786e35ca6 through 0ad645272d8b520d4c30eb877bc784f9e4af7441

configure.ac
Line 184: In the "case" branch, the -pthread(s) flag is appended to KEA_CXXFLAGS in two of the branches, the value appending being equal to the value of MULTITHREADING_FLAG which is also set in those branches. Suggest appending that value after the case statement with the single statement:

KEA_CXXFLAGS="$KEA_CXXFLAGS $MULTITHREADING_FLAG"

Line 1044: Why is MULTITHREADING_FLAG included in "LIBS". If required for linking, I would have thought it should be added to LDADD. (Although I do see that it was in LIBS before the change.) (Likewise at line 1100 - I would have thought that "-lpthread" should have been added to LIBS rather than LDFLAGS. But again, that was present before the change.)

doc/guide/install.xml
Line 112: Better is "If the header-only Boost error code files are not available or not wanted, the Boost system library is required"

Line 274: rather that say "same comment for --with-boot-libs", I would repeat that comment.

Line 354: Suggest changing "If you are" to "As another example, if you are". Also, change "library in /usr/pkg/lib" to "library in usr/pkg/lib, the following configure command could be used"

src/lib/cryptolink/openssl_link.cc
The RwLock class has more general application than just OpenSSL usage, so should be extracted out into a separate class and placed in src/lib/util/threads. There should be unit tests for it as well.

The Kea coding guidelines state that RwLock should be made non-copyable by inheriting from boost::noncopyable

Unless there is reason for it, the lock array "locks_" would be more logically declared as a static member variable of CryptoLinkImpl.

Is there a possibility that CRYPTO_NUM_locks() can return a value 0? if so, this should be checked for and the allocation of locks_ bypassed.

src/lib/util/process_spawn.cc
ProcessSpawnImpl::spawn. The last part of the method (where it has been established that we are in the parent process): wouldn't it be easier to reset the thread signal mask before trying to insert the PID/ProcessState into process_state_? That way there would be no need for the try/catch block.

src/lib/util/threads/thread.cc
Need to update the header of Thread::Impl, as we are now compelling with thread support.

Thread::Thread. As a thread inherits the signal mask of its parent, doesn't the declaration of "blocker" mean that all signals are disabled in the thread? Is this what we want? If so, a comment should be added explaining the reason.

Other

I don't know for mysql or postgresql...

The MySql and !PostgreSQL client libraries is more or less thread safe, albeit with Caveats:

This change will need a ChangeLog entry. Something like:

With the addition of a background thread for timeouts, ensure that the
cryptographic libraries and process spawning code are thread safe.

comment:8 Changed 4 years ago by fdupont

Before reading all your comments: did you review the trac4060a branch (only the obsolete trac4060 branch includes the openssl_link.cc changes)?

comment:9 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:10 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

I had looked at the trac4060 branch - I didn't notice the bit about trac4060a. Looking at the latter (reviewing commit ea7f498bdad5925bb2f625f7676793e86aafae4c):

configure.ac
Comment above still holds.

process_spawn.cc
Comment above still holds.

comment:11 in reply to: ↑ 10 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Replying to stephen:

I had looked at the trac4060 branch - I didn't notice the bit about trac4060a. Looking at the latter (reviewing commit ea7f498bdad5925bb2f625f7676793e86aafae4c):

configure.ac
Comment above still holds.

=> applied the line 184 suggestion. For the line 1044 I tried to keep the current code even I agree it doesn't look very clean (but it works).

process_spawn.cc
Comment above still holds.

=> if the sigmask is restored before the insert we have a race with the signal handler: the whole idea to mask the signal is to be sure the handler can't be called in parallel. Note the sigismember check is a part of this: the signal must be masked in the thread which may receive signals. So if the code doesn't seem very nice it is only because C++ lacks a "finally" construct.

What about a ChangeLog (the change could be user visible if it breaks something and the fact code must be thread safe is not a minor point).

comment:12 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit e7026936c64b51686c8b3e584e46a29a3a6a72fa

if the sigmask is restored before the insert we have a race with the signal handler

Good point.

So if the code doesn't seem very nice it is only because C++ lacks a "finally" construct.

I've lost count of the number of times I've wished for a "finally" construct in C++.

What about a ChangeLog (the change could be user visible if it breaks something and the fact code must be thread safe is not a minor point).

I've suggested a ChangeLog entry in an earlier comment.

All OK, please merge.

comment:13 Changed 4 years ago by fdupont

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

Merged. Creating a follow up for crypto and DB libraries. (#4118) Closing.

comment:14 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.