From 0bbf8f5d859b2e951e5be0464cde43dabe9867c0 Mon Sep 17 00:00:00 2001
From: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
Date: Tue, 2 Jan 2024 17:27:52 +0100
Subject: [PATCH] pool: remove IdleHandler from http and xroot movers

Motivation:
Two reasons why idle connection handler is not needed:

- pool provides `jtm` which can be use to achieve the same behavoir

- idle timeouts in general makes no sense with posix access, as
  multi-hour jobs can keep files open for the whole duration
  of the execution.

Modification:
Remove IdleHandler from Http/XrootdTransferService clases. Update
supporting code to obsolete the corresponding properties.

Result:
More predictable user job behaviour.

Ticket: #10533
Acked-by: Marina Sahakyan
Target: master
Require-book: no
Require-notes: yes
---
 .../xrootd/pool/XrootdTransferService.java    |  8 ++------
 .../dcache/http/HttpPoolRequestHandler.java   |  6 ------
 .../org/dcache/http/HttpTransferService.java  |  8 +-------
 .../pool/movers/NettyTransferService.java     | 20 +------------------
 .../org/dcache/pool/classic/pool.xml          |  4 ----
 skel/share/defaults/pool.properties           | 19 ++++--------------
 skel/share/services/pool.batch                |  4 ----
 7 files changed, 8 insertions(+), 61 deletions(-)

diff --git a/modules/dcache-xrootd/src/main/java/org/dcache/xrootd/pool/XrootdTransferService.java b/modules/dcache-xrootd/src/main/java/org/dcache/xrootd/pool/XrootdTransferService.java
index 79d6d47e9cc..90a5e6254e7 100644
--- a/modules/dcache-xrootd/src/main/java/org/dcache/xrootd/pool/XrootdTransferService.java
+++ b/modules/dcache-xrootd/src/main/java/org/dcache/xrootd/pool/XrootdTransferService.java
@@ -1,6 +1,6 @@
 /* dCache - http://www.dcache.org/
  *
- * Copyright (C) 2013-2023 Deutsches Elektronen-Synchrotron
+ * Copyright (C) 2013-2024 Deutsches Elektronen-Synchrotron
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Affero General Public License as
@@ -40,7 +40,6 @@
 import io.netty.channel.ChannelPipeline;
 import io.netty.channel.nio.NioEventLoopGroup;
 import io.netty.handler.logging.LoggingHandler;
-import io.netty.handler.timeout.IdleStateHandler;
 
 import java.io.IOException;
 import java.net.InetSocketAddress;
@@ -431,10 +430,7 @@ protected void initChannel(Channel ch) throws Exception {
             pipeline.addLast("plugin:" + plugin.getName(),
                   plugin.createHandler());
         }
-        pipeline.addLast("timeout", new IdleStateHandler(0,
-              0,
-              clientIdleTimeout,
-              clientIdleTimeoutUnit));
+
         pipeline.addLast("chunkedWriter", new ChunkedResponseWriteHandler());
 
         /*
diff --git a/modules/dcache/src/main/java/org/dcache/http/HttpPoolRequestHandler.java b/modules/dcache/src/main/java/org/dcache/http/HttpPoolRequestHandler.java
index 5a1239dd45c..3208ee7d039 100644
--- a/modules/dcache/src/main/java/org/dcache/http/HttpPoolRequestHandler.java
+++ b/modules/dcache/src/main/java/org/dcache/http/HttpPoolRequestHandler.java
@@ -58,7 +58,6 @@
 import io.netty.handler.codec.http.QueryStringDecoder;
 import io.netty.handler.timeout.IdleState;
 import io.netty.handler.timeout.IdleStateEvent;
-import io.netty.handler.timeout.IdleStateHandler;
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -797,11 +796,6 @@ private Object read(ChannelHandlerContext context, NettyTransferService<HttpProt
         long length = (upperRange - lowerRange) + 1;
 
         if (_useZeroCopy) {
-            // disable timeout manager as zero-copy can't keep idle counters in sync
-            var hasIdleStateHandler = context.channel().pipeline().get(IdleStateHandler.class) != null;
-            if (hasIdleStateHandler) {
-                context.channel().pipeline().remove(IdleStateHandler.class);
-            }
             return asFileRegion(file, lowerRange, length);
         }
         return new ReusableChunkedNioFile(file, lowerRange, length, _chunkSize);
diff --git a/modules/dcache/src/main/java/org/dcache/http/HttpTransferService.java b/modules/dcache/src/main/java/org/dcache/http/HttpTransferService.java
index b6823bde872..0d0999d35bc 100644
--- a/modules/dcache/src/main/java/org/dcache/http/HttpTransferService.java
+++ b/modules/dcache/src/main/java/org/dcache/http/HttpTransferService.java
@@ -1,6 +1,6 @@
 /* dCache - http://www.dcache.org/
  *
- * Copyright (C) 2013-2023 Deutsches Elektronen-Synchrotron
+ * Copyright (C) 2013-2024 Deutsches Elektronen-Synchrotron
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Affero General Public License as
@@ -38,7 +38,6 @@
 import io.netty.handler.codec.http.cors.CorsHandler;
 import io.netty.handler.logging.LoggingHandler;
 import io.netty.handler.stream.ChunkedWriteHandler;
-import io.netty.handler.timeout.IdleStateHandler;
 
 import java.net.InetSocketAddress;
 import java.net.SocketException;
@@ -166,11 +165,6 @@ protected void addChannelHandlers(ChannelPipeline pipeline) throws Exception {
         if (LOGGER.isDebugEnabled()) {
             pipeline.addLast("logger", new LoggingHandler());
         }
-        pipeline.addLast("idle-state-handler",
-              new IdleStateHandler(0,
-                    0,
-                    clientIdleTimeout,
-                    clientIdleTimeoutUnit));
         pipeline.addLast("chunkedWriter", new ChunkedWriteHandler());
         pipeline.addLast("keepalive", new KeepAliveHandler());
 
diff --git a/modules/dcache/src/main/java/org/dcache/pool/movers/NettyTransferService.java b/modules/dcache/src/main/java/org/dcache/pool/movers/NettyTransferService.java
index d104337974a..4f96ca81fce 100644
--- a/modules/dcache/src/main/java/org/dcache/pool/movers/NettyTransferService.java
+++ b/modules/dcache/src/main/java/org/dcache/pool/movers/NettyTransferService.java
@@ -1,6 +1,6 @@
 /* dCache - http://www.dcache.org/
  *
- * Copyright (C) 2013 - 2023 Deutsches Elektronen-Synchrotron
+ * Copyright (C) 2013 - 2024 Deutsches Elektronen-Synchrotron
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Affero General Public License as
@@ -192,24 +192,6 @@ public void setPostTransferService(
         this.postTransferService = postTransferService;
     }
 
-    public long getClientIdleTimeout() {
-        return clientIdleTimeout;
-    }
-
-    @Required
-    public void setClientIdleTimeout(long clientIdleTimeout) {
-        this.clientIdleTimeout = clientIdleTimeout;
-    }
-
-    public TimeUnit getClientIdleTimeoutUnit() {
-        return clientIdleTimeoutUnit;
-    }
-
-    @Required
-    public void setClientIdleTimeoutUnit(TimeUnit clientIdleTimeoutUnit) {
-        this.clientIdleTimeoutUnit = clientIdleTimeoutUnit;
-    }
-
     public long getConnectTimeout() {
         return connectTimeout;
     }
diff --git a/modules/dcache/src/main/resources/org/dcache/pool/classic/pool.xml b/modules/dcache/src/main/resources/org/dcache/pool/classic/pool.xml
index 465094e8871..5d69a55cabc 100644
--- a/modules/dcache/src/main/resources/org/dcache/pool/classic/pool.xml
+++ b/modules/dcache/src/main/resources/org/dcache/pool/classic/pool.xml
@@ -383,8 +383,6 @@
       <property name="postTransferService" ref="post-transfer-service"/>
       <property name="pnfsStub" ref="pnfsStub"/>
       <property name="threads" value="${pool.mover.xrootd.threads}"/>
-      <property name="clientIdleTimeout" value="${pool.mover.xrootd.timeout.idle}"/>
-      <property name="clientIdleTimeoutUnit" value="${pool.mover.xrootd.timeout.idle.unit}"/>
       <property name="connectTimeout" value="${pool.mover.xrootd.timeout.connect}"/>
       <property name="connectTimeoutUnit" value="${pool.mover.xrootd.timeout.connect.unit}"/>
       <property name="tpcClientChunkSize" value="#{ byteSizeParser.parse('${pool.mover.xrootd.tpc-client-chunk-size}') }"/>
@@ -459,8 +457,6 @@
         <property name="postTransferService" ref="post-transfer-service"/>
         <property name="threads" value="${pool.mover.http.threads}"/>
         <property name="chunkSize" value="${pool.mover.http.chunk-size}"/>
-        <property name="clientIdleTimeout" value="${pool.mover.http.timeout.idle}"/>
-        <property name="clientIdleTimeoutUnit" value="${pool.mover.http.timeout.idle.unit}"/>
         <property name="connectTimeout" value="${pool.mover.http.timeout.connect}"/>
         <property name="connectTimeoutUnit" value="${pool.mover.http.timeout.connect.unit}"/>
         <property name="doorStub" ref="doorStub"/>
diff --git a/skel/share/defaults/pool.properties b/skel/share/defaults/pool.properties
index 64e241701b8..48ea278ce5b 100644
--- a/skel/share/defaults/pool.properties
+++ b/skel/share/defaults/pool.properties
@@ -400,14 +400,6 @@ pool.mover.xrootd.plugins=
 #
 pool.mover.xrootd.tpc-authn-plugins=gsi,unix,ztn
 
-#  ---- xroot mover-idle timeout
-#
-#   Specifies the timeout before clients that connect to the
-#   pool request handler but don't open any files will be disconnected.
-#
-pool.mover.xrootd.timeout.idle = 300000
-(one-of?MILLISECONDS|SECONDS|MINUTES|HOURS|DAYS)pool.mover.xrootd.timeout.idle.unit=MILLISECONDS
-
 #  ---- xroot connect timeout
 #
 #   Timeout that the mover will wait for a client connection before
@@ -519,13 +511,6 @@ pool.mover.http.chunk-size = 8192
 (prefix)pool.mover.http.custom-response-header = HTTP headers that are always included in dCache responses
 pool.mover.http.custom-response-header!Server = dCache/${dcache.version}
 
-#  ---- HTTP client timeout
-#
-#   Period after which a client will be disconnected if the
-#   connection is idle (not reading or writing)
-#
-pool.mover.http.timeout.idle = 300
-(one-of?MILLISECONDS|SECONDS|MINUTES|HOURS|DAYS)pool.mover.http.timeout.idle.unit = SECONDS
 
 #  ---- HTTP connect timeout
 #
@@ -792,6 +777,10 @@ pool.info-request-handler.threads=4
 # Obsolete properties
 (obsolete)pool.cell.export = See pool.cell.consume
 (obsolete)pool.enable.remove-precious-files-on-delete = Precious replicas are always removed when file deleted in the namespace.
+(obsolete)pool.mover.xrootd.timeout.idle=The property is obsolete, use job timeout manager (jtm) instead.
+(obsolete)pool.mover.xrootd.timeout.idle.unit=The property is obsolete, use job timeout manager (jtm) instead.
+(obsolete)pool.mover.http.timeout.idle=The property is obsolete, use job timeout manager (jtm) instead.
+(obsolete)pool.mover.http.timeout.idle.unit=The property is obsolete, use job timeout manager (jtm) instead.
 
 
 
diff --git a/skel/share/services/pool.batch b/skel/share/services/pool.batch
index 8945d95f97d..76fe7c56d40 100644
--- a/skel/share/services/pool.batch
+++ b/skel/share/services/pool.batch
@@ -45,8 +45,6 @@ check -strong pool.mover.ftp.allow-incoming-connections
 check -strong pool.mover.ftp.mmap
 check -strong pool.mover.ftp.read-ahead
 check -strong pool.mover.xrootd.threads
-check -strong pool.mover.xrootd.timeout.idle
-check -strong pool.mover.xrootd.timeout.idle.unit
 check -strong pool.mover.xrootd.timeout.connect
 check -strong pool.mover.xrootd.timeout.connect.unit
 check -strong pool.mover.xrootd.read-reconnect-timeout
@@ -66,8 +64,6 @@ check -strong pool.mover.xrootd.security.tls.require-data
 check -strong pool.mover.xrootd.security.tls.require-gpf
 check -strong pool.mover.xrootd.security.tls.require-tpc
 check -strong pool.mover.http.threads
-check -strong pool.mover.http.timeout.idle
-check -strong pool.mover.http.timeout.idle.unit
 check -strong pool.mover.http.timeout.connect
 check -strong pool.mover.http.timeout.connect.unit
 check -strong pool.mover.http.chunk-size