HPCC-31130 Handle MP accept connection negotiations asynchronously #18273

jakesmith · 2024-02-02T15:04:12Z

And handoff slow/blocking connection negotiations to separate dedicated thread.

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

jakesmith · 2024-02-02T15:05:53Z

@mckellyln - please review.
It is worth noting, that the code in handleAcceptedSocket is an almost wholesale move of the code that used to be in CMPConnectThread::run()

github-actions · 2024-02-02T16:26:06Z

https://track.hpccsystems.com/browse/HPCC-31130
This pull request is already registered

mckellyln · 2024-02-06T14:27:51Z

system/mp/mpcomm.cpp

+        std::vector<Owned<ISocket>> slowClientsSocks;
+        CriticalSection crit;
+        Semaphore sem;
+        bool stopped = true;


Should stopped be std::atomic ?

probably, I'll change it.

mckellyln · 2024-02-06T17:35:29Z

system/mp/mpcomm.cpp

+                    {
+                        constexpr unsigned timeoutMs = 5000;
+                        Owned<ISocket> handledSock = sock.getClear();
+                        if (owner.handleAcceptedSocket(handledSock.getLink(), timeoutMs, false))


Why try for 5000 ms before handing off to async thread(s) ? Why not just always use async thread(s) ?
Maybe almost same thing if we change 5000 ms here to 100 ms or something very short ..

It does always use an async thread - this code is in in CMPConnectionThread::threadmain (a thread in the pool).

What it's doing here (within a thread of the pool), is handing this connection off, to outside the thread pool, to a separate 'slow' handler, so pool doesn't clog up.

jakesmith · 2024-02-07T13:25:48Z

@mckellyln - made 'stopped' an atomic, and see reply re. other question.

mckellyln · 2024-02-08T15:25:33Z

I've done some testing and this PR looks good, but I can still get some bad behavior when a rogue client connects to Thor and sends in various amounts of garbage data. I have created https://track.hpccsystems.com/browse/HPCC-31251 for additional work on this.

mckellyln

I approve this PR.

ghalliday · 2024-02-09T11:17:45Z

@jakesmith please can you add a summary of the solution to the jira.

jakesmith · 2024-02-09T12:04:02Z

@jakesmith please can you add a summary of the solution to the jira.

Added. Also noticed a problem with the way the optional config. option was being picked up in k8s. Addressed in new commit.
@ghalliday

ghalliday

@jakesmith A couple of problems and a few minor other comments.

system/mp/mpcomm.cpp

ghalliday · 2024-02-09T11:27:25Z

system/mp/mpcomm.cpp

+                    continue;
+                }
+
+                Linked<ISocket> sock = slowClientsSocks.back();


This needs to be protected within the critical section - otherwise it could get an invalid item. Theoretically the empty() call needs to be as well - on a chip with a very weak memory model..

I will put both in crit block.

ghalliday · 2024-02-09T11:30:06Z

system/mp/mpcomm.cpp

+        {
+            CriticalBlock b(crit);
+            for (auto &sock : slowClientsSocks)
+                sock->close();


Is it safe to call close() on a socket that is currently being listened to on another thread? (If stop is called while waiting for a slow response). From memory it can be complicated.

it will potentially currently being read from (CSocket::readtms) at this point, and on a ::poll call.

I think socket::close() will cause the poll to quit (I did test this scenario).
I thought it was a used technique elsewhere.

@mckellyln - what are your thoughts on this/interrupting the read?

reading up, I think it is unsafe, also, the handling of getting slowClientsSocks.back() in crit, then later calling pop_back() is unsafe is new items are being added.

I am going to remove the socket from the slowClientsSocks before calling handleAcceptedSocket.
It will mean it can't be interrupted and will timeout, but that's not really a problem, just a theoretical slightly longer delay during shutdown if happens to be on a stalling socket.

ghalliday · 2024-02-09T11:32:49Z

system/mp/mpcomm.cpp

+        void add(ISocket *sock) // NB: takes ownership
+        {
+            if (stopped)
+                return;


This will leak sock.
I'm not sure what is the cleanest pattern to implement this. Possibly pass an Owned or link the socket....

I think it's safe to assume it will get to the emplace_back once past this point (and thus own).
So a specific sock->Release here if stopped is a reasonable resolution.

ghalliday · 2024-02-09T12:11:21Z

system/mp/mpcomm.cpp

            running = false;
            listensock->cancel_accept();
+            if (listen)
+            {
+                if (!threadPool->joinAll(true, 1000*60*5))


Worth commenting that stop the listening threads first in case any get added to the stop processor?

I'll move the join for this (CMPConnectThread) accept thread ahead of this, and add a comment.

ghalliday · 2024-02-09T12:14:24Z

system/mp/mpcomm.cpp

@@ -2178,194 +2265,272 @@ void CMPConnectThread::startPort(unsigned short port)
    if (!listen)
        return;
    running = true;
+    if (acceptThreadPoolSize)
+    {
+        class CFactory : public CInterfaceOf<IThreadFactory>


CFactory: The name could be more descriptive - what is it a factory for?

Will rename to CMPConnectThreadFactory.
I do tend to use generic names if inline classes are used and therefore implicitly relevant to the surrounding code.

system/mp/mpcomm.cpp

ghalliday · 2024-02-09T12:20:15Z

system/mp/mpcomm.cpp

            running = false;
            listensock->cancel_accept();
+            if (listen)
+            {
+                if (!threadPool->joinAll(true, 1000*60*5))


threadPool could be null if the pool size is set to 0

yes, will add a check.

ghalliday · 2024-02-09T12:21:52Z

system/mp/mpcomm.cpp

+                        Owned<ISocket> handledSock = sock.getClear();
+                        if (owner.handleAcceptedSocket(handledSock.getLink(), timeoutMs, false))
+                        {
+                            // handoff to slowClientProcessor, which will retry for standard CONFIRM_TIMEOUT period                            owner.slowClientProcessor.add(sock.getClear());


spurious (almost) copy of the next line on the end of the comment.

ah yes, couldn't even spot it in vscode at first!
Will remove.

ghalliday · 2024-02-09T12:23:24Z

system/mp/mpcomm.cpp

+                    {
+                        constexpr unsigned timeoutMs = 5000;
+                        Owned<ISocket> handledSock = sock.getClear();
+                        if (owner.handleAcceptedSocket(handledSock.getLink(), timeoutMs, false))


Not worth changing, but it is counter-intuitive that a function called handleAcceptedSocket() returns true if it fails to handle it.

agree, and pondered this, but really true|false is insufficient. As it stands returning false means it did not timeout, but it doesn't mean it succeeded. Various failure conditions also return false.
So really it should return a state, but I only care about 1 of them (timeout).

jakesmith · 2024-02-09T13:51:51Z

@ghalliday - please see new commit.

ghalliday

@jakesmith please squash

And handoff slow/blocking connection negotiations to separate dedicated thread. Signed-off-by: Jake Smith <[email protected]>

jakesmith · 2024-02-09T16:23:36Z

@jakesmith please squash

@ghalliday - done

jakesmith requested a review from mckellyln February 2, 2024 15:04

jakesmith force-pushed the HPCC-31130-mp-accept-threadpool branch from 19304a9 to 06dfbfb Compare February 2, 2024 15:29

mckellyln reviewed Feb 6, 2024

View reviewed changes

jakesmith requested a review from mckellyln February 7, 2024 13:25

jakesmith force-pushed the HPCC-31130-mp-accept-threadpool branch from d920488 to 873ebe1 Compare February 8, 2024 16:43

mckellyln approved these changes Feb 8, 2024

View reviewed changes

ghalliday reviewed Feb 9, 2024

View reviewed changes

jakesmith requested a review from ghalliday February 9, 2024 13:51

ghalliday approved these changes Feb 9, 2024

View reviewed changes

HPCC-31130 Handle MP accept connection negotiations asynchronously

6c13c81

And handoff slow/blocking connection negotiations to separate dedicated thread. Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the HPCC-31130-mp-accept-threadpool branch from 19ae797 to 6c13c81 Compare February 9, 2024 15:46

ghalliday approved these changes Feb 9, 2024

View reviewed changes

ghalliday merged commit abda0ef into hpcc-systems:candidate-9.4.x Feb 9, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31130 Handle MP accept connection negotiations asynchronously #18273

HPCC-31130 Handle MP accept connection negotiations asynchronously #18273

jakesmith commented Feb 2, 2024 •

edited

Loading

jakesmith commented Feb 2, 2024

github-actions bot commented Feb 2, 2024

mckellyln Feb 6, 2024

jakesmith Feb 7, 2024

mckellyln Feb 6, 2024

jakesmith Feb 7, 2024

jakesmith commented Feb 7, 2024

mckellyln commented Feb 8, 2024

mckellyln left a comment

ghalliday commented Feb 9, 2024

jakesmith commented Feb 9, 2024

ghalliday left a comment

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

ghalliday Feb 9, 2024

jakesmith Feb 9, 2024

jakesmith commented Feb 9, 2024

ghalliday left a comment

jakesmith commented Feb 9, 2024

HPCC-31130 Handle MP accept connection negotiations asynchronously #18273

HPCC-31130 Handle MP accept connection negotiations asynchronously #18273

Conversation

jakesmith commented Feb 2, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

jakesmith commented Feb 2, 2024

github-actions bot commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Feb 7, 2024

mckellyln commented Feb 8, 2024

mckellyln left a comment

Choose a reason for hiding this comment

ghalliday commented Feb 9, 2024

jakesmith commented Feb 9, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Feb 9, 2024

ghalliday left a comment

Choose a reason for hiding this comment

jakesmith commented Feb 9, 2024

jakesmith commented Feb 2, 2024 •

edited

Loading