topic/jsiwek/thread-termination

Description

The change in this branch should fix the case where the last remaining done/killed thread never got processed (main thread never received pending messages from it or joined/deleted it) until Bro terminates. Which was problematic if the termination condition depended on processing messages from the last remaining thread.

The new code's logic is contrary to what it used to be, but I can't figure out what the old was trying to accomplish and think it could only have caused problems.

Environment

None

Activity

Show:
Robin Sommer
October 30, 2013, 12:04 PM

I looked up when the original "&& ! Killed()" code got introduced, that was in 743fc1680dc9d4c04f38ca80c7ef4e5b88e8f4cb and the commit message points to BIT-858. Can you take a look and double-check that the problem described there is still addressed with the new version to be sure we don't introduce a regression? (Not immediately sure if we have a test that covers that).

Robin Sommer
October 30, 2013, 1:31 PM

I've merged it but reopening for the double-check.

Jon Siwek
October 30, 2013, 3:56 PM

testing/btst/scripts/base/frameworks/input/missing-file.bro seems to at least be checking part of the problem mentioned in BIT-858. And I don't think that this conflicts with what was addressed there. Here's an abbreviated history of "thread termination":

743fc1680dc9d4c04f38ca80c7ef4e5b88e8f4cb

  • threading::Manager:rocess() and threading::Manager::NextTimestamp() both check for "&& ! t->Killed()"

  • The assumption is that you're not allowed to read from a "dead" thread's queue and threads are only cleaned up in threading::Manager::Terminate()

38e1dc9ca47d97508276a2f7192c5353bb8e6837

  • threading::Manager:rocess() can now also clean up dead threads

b947394990720032ac7f374f7c9d1902ed4485b9

  • Reading from a dead thread's queue is now supported

  • "&& ! t->Killed()" check is removed from threading::Manager:rocess() to allow flushing out a dead thread's queue before cleaning it up, but the check still remains in threading::Manager::NextTimestamp()

To me, looks like the NextTimestamp code just didn't evolve w/ the rest.

Assignee

Robin Sommer

Reporter

Jon Siwek

Labels

None

External issue ID

None

Components

Fix versions

Affects versions

Priority

Normal
Configure