Fixes #1145: Implement idle connection timeout for tcpListener #1148

kgiusti · 2023-06-30T13:24:58Z

No description provided.

codecov · 2023-06-30T13:34:28Z

Codecov Report

Merging #1148 (216f557) into main (7895c27) will increase coverage by 0.03%.
The diff coverage is 86.07%.

❗ Current head 216f557 differs from pull request most recent head cfb59de. Consider uploading reports for the commit cfb59de to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1148      +/-   ##
==========================================
+ Coverage   77.89%   77.92%   +0.03%     
==========================================
  Files         238      239       +1     
  Lines       60659    60741      +82     
  Branches     5576     5582       +6     
==========================================
+ Hits        47248    47333      +85     
  Misses      10779    10779              
+ Partials     2632     2629       -3

Flag	Coverage Δ
pysystemtests	`87.45% <91.66%> (+0.02%)`	⬆️
pyunittests	`54.51% <ø> (ø)`
systemtests	`71.59% <80.00%> (+<0.01%)`	⬆️
unittests	`27.20% <10.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
calculator	`30.80% <10.00%> (-0.02%)`	⬇️
systemtests	`78.51% <87.17%> (+0.01%)`	⬆️

ganeshmurthy · 2023-06-30T13:34:36Z

Can we please change the commit message to "Fixes #1134:......"
That way we will have link to the issue right on the commit which will help us navigate to the issue quickly and merging this PR will close the issue automatically

ganeshmurthy · 2023-06-30T13:42:12Z

python/skupper_router/management/skrouter.json

@@ -1254,6 +1254,13 @@
                    "type": ["up", "down"],
                    "description": "The operational status of TCP socket listener: up - the service is active and incoming connections are permitted; down - the service is not active and incoming connection attempts will be refused.",
                    "create": false
+                },
+                "idleTimeoutSeconds": {


We used to have an attribute called dataConnectionCount which used to be in the connector entity and @grs requested that it be moved to the router entity which we did. @grs, I just wanted to get your thoughts on whether this idleTimeoutSeconds should be in the router entity as well ?

From a code standpoint it's easier to have it in the listener. Having it in the listener also allows us to disable/adjust it per-service type. More flexibility. You'll also notice that the AMQP listener and connector management object have the same 'idleTimeoutSeconds' attribute, so adding it to the tcpListener is consistent with that.

I don't see a compelling advantage to having a single global value for all services. Is there a use-case where it makes sense?

I did not realize that we are only terminating idle connections on the client side but not on the server side. Terminating the client side connections will automatically terminate the server side connections as well, so we should be ok. I cannot think of a case where idle server side connections will need to be terminated.

The counter to that is whether we really envisage users wanting to set different values for this? What will the default be? Could there be a way to configure the default (i.e. a way to change it for all listeners in one place instead of changing the configuration of each individual listener)?

What advice would we give users about when to use this option and what to set it to? And can we make it so that in our opinion the majority of users can ignore it?

python/skupper_router/management/skrouter.json

kgiusti · 2023-06-30T14:02:18Z

Can we please change the commit message to "Fixes #1134:......" That way we will have link to the issue right on the commit which will help us navigate to the issue quickly and merging this PR will close the issue automatically

I can update the git commit message, but I've also set the issue in the github Development attribute associated with this pull request which will automagically close the related issue. See left sidebar.

ganeshmurthy · 2023-06-30T15:15:07Z

Can we please change the commit message to "Fixes #1134:......" That way we will have link to the issue right on the commit which will help us navigate to the issue quickly and merging this PR will close the issue automatically

I can update the git commit message, but I've also set the issue in the github Development attribute associated with this pull request which will automagically close the related issue. See left sidebar.

Yes, update the commit message please. The commit message, when referencing an issue, would be nice if we start the commit message with "Fixes #yyy: ...." as we agreed to do here - https://github.com/skupperproject/skupper-router/blob/main/CONTRIBUTING.adoc

ganeshmurthy · 2023-06-30T15:34:49Z

src/adaptors/tcp/tcp_adaptor.c

@@ -1089,6 +1115,21 @@ static void handle_connection_event(pn_event_t *e, qd_server_t *qd_server, void
    case PN_RAW_CONNECTION_WAKE: {
        qd_log(LOG_TCP_ADAPTOR, QD_LOG_DEBUG, "[C%" PRIu64 "] PN_RAW_CONNECTION_WAKE %s", conn->conn_id,
               qdr_tcp_connection_role_name(conn));
+        if (CLEAR_ATOMIC_FLAG(&conn->check_idle_conn)) {


Can we do this bit of code only if conn->ingress is true ?

Good idea! How about we check for existence of the idle_timer? The patch already does that elsewhere since the idle_timer is only allocated if idleTimeoutSeconds != 0.

ganeshmurthy · 2023-06-30T15:35:42Z

src/adaptors/tcp/tcp_adaptor.c

@@ -1209,9 +1250,14 @@ static qdr_tcp_connection_t *qdr_tcp_connection(qd_tcp_listener_t *listener, qd_
    assert(tcp_stats);
    assert(server);

+    if (tc->config->adaptor_config->idle_timeout) {


Move this into the above if (listener) so we can have idle timers only for connections on the listener side ?

We may get a request to add timeout support for the connector side as well. Still not carved in stone but if so I'd rather not conventionalize the code on the type of configuration. We'll just have to revert that change if so.

kgiusti · 2023-06-30T17:13:38Z

Can we please change the commit message to "Fixes #1134:......" That way we will have link to the issue right on the commit which will help us navigate to the issue quickly and merging this PR will close the issue automatically

I can update the git commit message, but I've also set the issue in the github Development attribute associated with this pull request which will automagically close the related issue. See left sidebar.

Yes, update the commit message please. The commit message, when referencing an issue, would be nice if we start the commit message with "Fixes #yyy: ...." as we agreed to do here - https://github.com/skupperproject/skupper-router/blob/main/CONTRIBUTING.adoc

Hmmm... ok so I changed the commit message as described (needed to squash/rebase) and I've updated the name of this PR, but now the link to the actual issue in the Development field has gone away. Is this the correct message syntax?

ganeshmurthy · 2023-06-30T17:28:30Z

Can we please change the commit message to "Fixes #1134:......"

I put in the wrong issue #, the correct issue # is #1145
Even after squash rebase, I see the commit message say "ISSUE-1134:
The commit message should say "Fixes #1145: ...."
My apologies.

…istener

kgiusti · 2023-07-03T21:33:33Z

Marking this as Draft:

This approach won't scale effectively due to its timer-per-connection implementation. Our timer implementation is the problem: it maintains a linear sorted linked list of timers. As the number of connections scale so do the timers. Scheduling a timer becomes prohibitively expensive timewise as the number of connections scales. In my tests when I scale to 65K timers scheduling a timer can take several milliseconds each (lock held).

A better approach might be to have a timer-per listener and sweep the list of associated connections when the timer expires. This is similar to the approach taken for stuck delivery detection.

I'll need to think more about this.

ganeshmurthy · 2024-02-20T15:26:56Z

Temporarily closing this PR. We will revisit when this becomes relevant again.

kgiusti requested review from ted-ross and ganeshmurthy June 30, 2023 13:24

ganeshmurthy reviewed Jun 30, 2023

View reviewed changes

python/skupper_router/management/skrouter.json Show resolved Hide resolved

kgiusti linked an issue Jun 30, 2023 that may be closed by this pull request

Connection Idle timeout feature for TCP Adaptor #1145

Open

kgiusti requested review from grs, ajssmith and nluaces June 30, 2023 14:03

ganeshmurthy reviewed Jun 30, 2023

View reviewed changes

kgiusti removed a link to an issue Jun 30, 2023

Connection Idle timeout feature for TCP Adaptor #1145

Open

kgiusti force-pushed the ISSUE-1145 branch from 216f557 to 0987f32 Compare June 30, 2023 17:10

kgiusti changed the title ~~ISSUE-1134: Implement idle connection timeout for tcpListener~~ Fixes #1134: Implement idle connection timeout for tcpListener Jun 30, 2023

Fixes skupperproject#1145: Implement idle connection timeout for tcpL…

cfb59de

…istener

kgiusti force-pushed the ISSUE-1145 branch from 0987f32 to cfb59de Compare July 3, 2023 16:00

kgiusti changed the title ~~Fixes #1134: Implement idle connection timeout for tcpListener~~ Fixes #1145: Implement idle connection timeout for tcpListener Jul 3, 2023

kgiusti linked an issue Jul 3, 2023 that may be closed by this pull request

Connection Idle timeout feature for TCP Adaptor #1145

Open

kgiusti marked this pull request as draft July 3, 2023 21:24

nluaces mentioned this pull request Jul 10, 2023

Set idleTimeoutSeconds values from Skupper command line skupperproject/skupper#1167

Closed

ganeshmurthy closed this Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1145: Implement idle connection timeout for tcpListener #1148

Fixes #1145: Implement idle connection timeout for tcpListener #1148

kgiusti commented Jun 30, 2023

codecov bot commented Jun 30, 2023 •

edited

Loading

ganeshmurthy commented Jun 30, 2023

ganeshmurthy Jun 30, 2023

kgiusti Jun 30, 2023 •

edited

Loading

ganeshmurthy Jun 30, 2023

grs Jul 10, 2023

kgiusti commented Jun 30, 2023

ganeshmurthy commented Jun 30, 2023

ganeshmurthy Jun 30, 2023

kgiusti Jun 30, 2023

ganeshmurthy Jun 30, 2023

ganeshmurthy Jun 30, 2023

kgiusti Jun 30, 2023

kgiusti commented Jun 30, 2023

ganeshmurthy commented Jun 30, 2023

kgiusti commented Jul 3, 2023

ganeshmurthy commented Feb 20, 2024

Fixes #1145: Implement idle connection timeout for tcpListener #1148

Fixes #1145: Implement idle connection timeout for tcpListener #1148

Conversation

kgiusti commented Jun 30, 2023

codecov bot commented Jun 30, 2023 • edited Loading

Codecov Report

ganeshmurthy commented Jun 30, 2023

Choose a reason for hiding this comment

kgiusti Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgiusti commented Jun 30, 2023

ganeshmurthy commented Jun 30, 2023

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

kgiusti commented Jun 30, 2023

ganeshmurthy commented Jun 30, 2023

kgiusti commented Jul 3, 2023

ganeshmurthy commented Feb 20, 2024

codecov bot commented Jun 30, 2023 •

edited

Loading

kgiusti Jun 30, 2023 •

edited

Loading