Fix SIGTERM can't terminate PubSub::listen issue by add cancellation token support. #606

liuh-80 · 2022-04-19T08:14:54Z

Why I did it

There are infinite loops inside PubSub::listen() method, so application using this method can't handle SIGTERM correctly.
https://github.com/Azure/sonic-swss-common/issues/603

How I did it

Add following class:
1. CancellationToken: this class will help exist the infinite loops when SIGTERM or other signal happen.
2. SignalHandlerHelper: Provide a native signal handler.

How to verify it

manually test.
Pass all test case.

Which release branch to backport (provide reason below if selected)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

common/cancellationtoken.h

common/cancellationtoken.cpp

common/cancellationtoken.h

common/signalhandlerhelper.cpp

stepanblyschak · 2022-04-20T08:28:27Z

common/signalhandlerhelper.cpp

+
+void SignalHandlerHelper::RegisterSignalHandler(int signalNumber)
+{
+    signal(signalNumber, SignalHandlerHelper::OnSignal);


Does it override the signal handler defined by the application? In case of python, does it mean that the user defined signal handler in python won't be executed?

Yes, because linux can only have 1 signal handler per-application.
If customer need define their signal handler, then customer also need cancel the token when signal happen in their handler, this class just a helper class.

common/select.cpp

common/configdb.h

common/select.cpp

qiluo-msft · 2022-04-22T07:00:39Z

ret = poll_descriptors(c, timeout);

Did you change old behavior?

In reply to: 1106076032

Refers to: common/select.cpp:166 in 037283c. [](commit_id = 037283c, deletion_comment = False)

common/select.h

liuh-80 · 2022-04-22T07:20:27Z

ret = poll_descriptors(c, timeout);
Did you change old behavior?

Refers to: common/select.cpp:166 in 037283c. [](commit_id = 037283c, deletion_comment = False)

No, I didn't. when timeout happen, current code still will try read again.

qiluo-msft · 2022-04-25T20:58:41Z

ret = poll_descriptors(c, timeout);

For non terminating signals (like siguser), original behavior is keeping while loop, but the new behavior is return SIGNALINTR. I think this is unexpected.

In reply to: 1106076032

Refers to: common/select.cpp:166 in 037283c. [](commit_id = 037283c, deletion_comment = False)

liuh-80 · 2022-04-27T06:59:54Z

ret = poll_descriptors(c, timeout);
For non terminating signals (like siguser), original behavior is keeping while loop, but the new behavior is return SIGNALINTR. I think this is unexpected.

In reply to: 1106076032

Refers to: common/select.cpp:166 in 037283c. [](commit_id = 037283c, deletion_comment = False)

Will discussion offline, the reason is:

without a customize signal handler we don't know which signal break the epoll_wait.
When poll_descriptors break by siguser, it will return to python code and python code will re-run poll_descriptors method, so from the python code side, the behavior not change.

However. there are some UT failed, seems related with this change, will check reason.

Update: in latest code, only sigint can break the while loop, all other signal will not break the while loop.

liuh-80 · 2022-04-28T06:41:05Z

/azp run

azure-pipelines · 2022-04-28T06:41:16Z

Azure Pipelines successfully started running 1 pipeline(s).

liuh-80 · 2022-04-28T07:44:43Z

Add SignalHandlerHelper class back because:

There are UT break because in last version any signal will break poll_descriptors() method.
So the solution should only allow SIGTERM to break poll_descriptors().
However, according to linux signal system, it's impossible to check which signal happening when there is no signal handler.

So we have to add this class back.

Also here is demo code in python3:

from swsscommon import swsscommon
swsscommon.SignalHandlerHelper.registerSignalHandler(swsscommon.SIGNAL_INT)
c=swsscommon.ConfigDBConnector()
c.subscribe('A', lambda a: None)
c.connect()
c.listen(None)
^C>>>

common/pubsub.cpp

common/pubsub.h

common/signalhandlerhelper.cpp

common/select.cpp

liuh-80 · 2022-05-11T01:17:55Z

/azp run

azure-pipelines · 2022-05-11T01:18:07Z

Azure Pipelines successfully started running 1 pipeline(s).

liuh-80 · 2022-05-11T06:02:37Z

Code coverage does not reach target, will add UT to cover new code.

…ystemd (#2133)" (#2161) This reverts commit 23e9398. - What I did Revert "[scripts/fast-reboot] Shutdown remaining containers through systemd (#2133)" This reverted PR is part of a story that refactors warm/fast shutdown sequence to gracefully stop services instead of killing them without any ordering and dependency requirements which creates several issues and is error prone for the future. This PR must come together with sonic-net/sonic-buildimage#10510. However, #10510 is blocked due to an issue in swss-common sonic-net/sonic-swss-common#603 And a fix by MSFT is in review sonic-net/sonic-swss-common#606 I am reverting it because its dependency is still blocked and we cannot update submodule pointer. Once the dependency of the reverted PR is resolved, it shall be re-committed.

…ystemd (#2133)" (#2166) - What I did This reverts commit a5f55aa. Revert "[scripts/fast-reboot] Shutdown remaining containers through systemd (#2133)" This reverted PR is part of a story that refactors warm/fast shutdown sequence to gracefully stop services instead of killing them without any ordering and dependency requirements which creates several issues and is error prone for the future. This PR must come together with sonic-net/sonic-buildimage#10510. However, #10510 is blocked due to an issue in swss-common sonic-net/sonic-swss-common#603 And a fix by MSFT is in review sonic-net/sonic-swss-common#606 I am reverting it because its dependency is still blocked and we cannot update submodule pointer. Once the dependency of the reverted PR is resolved, it shall be re-committed. - How I did it git revert a5f55aa - How to verify it Run tests

liuh-80 · 2022-05-16T08:42:10Z

@stepanblyschak, could you please check this PR again? all issue fixed.

common/pubsub.cpp

common/signalhandlerhelper.cpp

qiluo-msft · 2022-05-16T19:12:04Z

ret = poll_descriptors(c, timeout);

Could you fix for non terminating signals (like siguser)?

In reply to: 1109033970

Refers to: common/select.cpp:166 in 037283c. [](commit_id = 037283c, deletion_comment = False)

liuh-80 · 2022-05-18T01:21:48Z

ret = poll_descriptors(c, timeout);
Could you fix for non terminating signals (like siguser)?

In reply to: 1109033970

Refers to: common/select.cpp:166 in 037283c. [](commit_id = 037283c, deletion_comment = False)

This already fixed, currently only sigint can break the while loop in select.cpp, all other signal will not break while loop.

…token support. (sonic-net#606) Why I did it There are infinite loops inside PubSub::listen() method, so application using this method can't handle SIGTERM correctly. sonic-net#603 How I did it Add following class: 1. CancellationToken: this class will help exist the infinite loops when SIGTERM or other signal happen. 2. SignalHandlerHelper: Provide a native signal handler. How to verify it 1. manually test. 2. Pass all test case.

…ystemd (#2133)" (#2161) This reverts commit 23e9398. - What I did Revert "[scripts/fast-reboot] Shutdown remaining containers through systemd (#2133)" This reverted PR is part of a story that refactors warm/fast shutdown sequence to gracefully stop services instead of killing them without any ordering and dependency requirements which creates several issues and is error prone for the future. This PR must come together with sonic-net/sonic-buildimage#10510. However, #10510 is blocked due to an issue in swss-common sonic-net/sonic-swss-common#603 And a fix by MSFT is in review sonic-net/sonic-swss-common#606 I am reverting it because its dependency is still blocked and we cannot update submodule pointer. Once the dependency of the reverted PR is resolved, it shall be re-committed.

liuh-80 added 5 commits April 19, 2022 07:46

Add cancellation token

15fb77d

Fix code issue

5f31e4b

Improve code

0aebb29

Fix code issue

a0f7835

Fix code issue

f0d849d

liuh-80 mentioned this pull request Apr 19, 2022

There is a problem that hostcfgd blocks forever and does not react to SIGTERM causing a delay in warm boot. #603

Closed

liuh-80 changed the title ~~Add cancellation token~~ Fix SIGTERM can't terminate PubSub::listen issue by add cancellation token support. Apr 19, 2022

liuh-80 added 2 commits April 19, 2022 14:45

Improve code

ea88a6f

Fix new method name conflict issue

876c421

liuh-80 marked this pull request as ready for review April 20, 2022 08:41

stepanblyschak reviewed Apr 20, 2022

View reviewed changes

liuh-80 requested a review from qiluo-msft April 21, 2022 02:11

liuh-80 added 2 commits April 21, 2022 05:43

Fix PR comments

30b4ec3

Simplify solution by only retry in python code.

037283c

qiluo-msft reviewed Apr 22, 2022

View reviewed changes

common/configdb.h Outdated Show resolved Hide resolved

qiluo-msft reviewed Apr 22, 2022

View reviewed changes

common/configdb.h Outdated Show resolved Hide resolved

qiluo-msft reviewed Apr 22, 2022

View reviewed changes

common/select.cpp Show resolved Hide resolved

qiluo-msft reviewed Apr 22, 2022

View reviewed changes

common/select.h Outdated Show resolved Hide resolved

Fix PR comments

04dbb8e

Fix PR comments

84f0dcc

Fix UT break by add signal handler back

27195f7

liuh-80 added 2 commits April 28, 2022 08:19

Fix code issue

d8af1ec

Fix python code issue

9241382