MDEV-21101 unexpected wait_timeout with pool-of-threads

Due to restricted size of the threadpool, execution of client queries can
be delayed (queued) for a while. This delay was interpreted as client
inactivity, and connection is closed, if client idle time + queue time
exceeds wait_timeout.

But users did not expect queue time to be included into wait_timeout.

This patch changes the behavior. We don't close connection anymore,
if there is some unread data present on connection,
even if wait_timeout is exceeded. Unread data means that client
was not idle, it sent a query, which we did not have time to process yet.

include/violite.h

@@ -110,9 +110,7 @@ my_bool vio_peer_addr(Vio *vio, char *buf, uint16 *port, size_t buflen);
 /* Wait for an I/O event notification. */
 int vio_io_wait(Vio *vio, enum enum_vio_io_event event, int timeout);
 my_bool vio_is_connected(Vio *vio);
-#ifndef DBUG_OFF
 ssize_t vio_pending(Vio *vio);
-#endif
 /* Set timeout for a network operation. */
 extern int vio_timeout(Vio *vio, uint which, int timeout_sec);
 extern void vio_set_wait_callback(void (*before_wait)(void),

mysql-test/main/mdev-21101.opt

@@ -0,0 +1 @@
+--thread-handling=pool-of-threads

mysql-test/main/mdev-21101.result

@@ -0,0 +1,43 @@
+SELECT
+@@global.wait_timeout, @@global.thread_pool_max_threads, @@global.thread_pool_size,
+@@global.thread_pool_oversubscribe,  @@global.thread_pool_stall_limit
+INTO
+@_wait_timeout,@_thread_pool_max_threads,@_thread_pool_size,
+@_thread_pool_oversubscribe,@_thread_pool_stall_limit;
+SET @@global.wait_timeout=1,
+@@global.thread_pool_max_threads=2,
+@@global.thread_pool_size=1,
+@@global.thread_pool_oversubscribe=1,
+@@global.thread_pool_stall_limit=10;
+connect  c1, localhost, root,,;
+connect  c2, localhost, root,,;
+connect  c3, localhost, root,,;
+connection c1;
+select sleep(1.1);
+connection c2;
+select sleep(1.1);
+connection c3;
+select sleep(1.1);
+connection default;
+select sleep(1.1);
+connection c1;
+sleep(1.1)
+0
+connection c2;
+sleep(1.1)
+0
+connection c3;
+sleep(1.1)
+0
+connection default;
+sleep(1.1)
+0
+disconnect c1;
+disconnect c2;
+disconnect c3;
+connection default;
+SET @@global.wait_timeout=@_wait_timeout,
+@@global.thread_pool_max_threads=@_thread_pool_max_threads,
+@@global.thread_pool_size=@_thread_pool_size,
+@@global.thread_pool_oversubscribe=@_thread_pool_oversubscribe,
+@@global.thread_pool_stall_limit=@_thread_pool_stall_limit;

mysql-test/main/mdev-21101.test

@@ -0,0 +1,53 @@
+# Test that wait_timeout does not cause connection to be closed, when connection is delayed due to
+# threadpool internal problems, e.g misconfiguration - too few threads and queueing.
+# So if client did not cause wait_timeout, do not report it either.
+# See MDEV-21101 for details.
+
+# Intentionally misconfigure threadpool to have at most 1 or 2 threads (
+# depends on the implementation). Use minimal wait_timeout, do some slow queries from
+# different connections simultaneously, to force queueing occurs.
+# Verify connections are intact, even if queueing time exceeds wait_timeout
+
+SELECT
+  @@global.wait_timeout, @@global.thread_pool_max_threads, @@global.thread_pool_size,
+  @@global.thread_pool_oversubscribe,  @@global.thread_pool_stall_limit
+INTO
+  @_wait_timeout,@_thread_pool_max_threads,@_thread_pool_size,
+  @_thread_pool_oversubscribe,@_thread_pool_stall_limit;
+
+SET @@global.wait_timeout=1,
+    @@global.thread_pool_max_threads=2,
+    @@global.thread_pool_size=1,
+    @@global.thread_pool_oversubscribe=1,
+    @@global.thread_pool_stall_limit=10;
+ 
+--connect (c1, localhost, root,,)
+--connect (c2, localhost, root,,)
+--connect (c3, localhost, root,,)
+--connection c1
+--send select sleep(1.1)
+--connection c2
+--send select sleep(1.1)
+--connection c3
+--send select sleep(1.1)
+--connection default
+--send select sleep(1.1)
+--connection c1
+--reap
+--connection c2
+--reap
+--connection c3
+--reap
+--connection default
+--reap
+--disconnect c1
+--disconnect c2
+--disconnect c3
+--connection default
+
+SET @@global.wait_timeout=@_wait_timeout,
+    @@global.thread_pool_max_threads=@_thread_pool_max_threads,
+    @@global.thread_pool_size=@_thread_pool_size,
+    @@global.thread_pool_oversubscribe=@_thread_pool_oversubscribe,
+    @@global.thread_pool_stall_limit=@_thread_pool_stall_limit;
+ 

sql/sys_vars.cc

@@ -3609,6 +3609,12 @@ static bool fix_tp_min_threads(sys_var *, THD *, enum_var_type)
 
 static bool check_threadpool_size(sys_var *self, THD *thd, set_var *var)
 {
+
+#ifdef _WIN32
+  if (threadpool_mode != TP_MODE_GENERIC)
+    return false;
+#endif
+
   ulonglong v= var->save_result.ulonglong_value;
   if (v > threadpool_max_size)
   {

sql/threadpool.h

@@ -74,6 +74,7 @@ enum TP_STATE
 {
   TP_STATE_IDLE,
   TP_STATE_RUNNING,
+  TP_STATE_PENDING
 };
 
 /*

sql/threadpool_common.cc

@@ -468,11 +468,25 @@ void tp_timeout_handler(TP_connection *c)
 {
   if (c->state != TP_STATE_IDLE)
     return;
-  THD *thd=c->thd;
+  THD *thd= c->thd;
   mysql_mutex_lock(&thd->LOCK_thd_kill);
+  Vio *vio= thd->net.vio;
+  if (vio && (vio_pending(vio) > 0 || vio->has_data(vio)) &&
+      c->state == TP_STATE_IDLE)
+  {
+    /*
+     There is some data on that connection, i.e
+     i.e there was no inactivity timeout.
+     Don't kill.
+    */
+    c->state= TP_STATE_PENDING;
+  }
+  else if (c->state == TP_STATE_IDLE)
+  {
     thd->set_killed_no_mutex(KILL_WAIT_TIMEOUT);
     c->priority= TP_PRIORITY_HIGH;
     post_kill_notification(thd);
+  }
   mysql_mutex_unlock(&thd->LOCK_thd_kill);
 }
 

sql/threadpool_generic.cc

@@ -591,11 +591,8 @@ static void timeout_check(pool_timer_t *timer)
   THD *thd;
   while ((thd=it++))
   {
-    if (thd->net.reading_or_writing != 1)
-      continue;
- 
     TP_connection_generic *connection= (TP_connection_generic *)thd->event_scheduler.data;
-    if (!connection)
+    if (!connection || connection->state != TP_STATE_IDLE)
     {
       /* 
         Connection does not have scheduler data. This happens for example

vio/vio_priv.h

@@ -33,6 +33,7 @@ my_bool vio_is_connected_pipe(Vio *vio);
 int vio_close_pipe(Vio * vio);
 int cancel_io(HANDLE handle, DWORD thread_id);
 int vio_shutdown_pipe(Vio *vio,int how);
+uint vio_pending_pipe(Vio* vio);
 #endif
 
 #ifdef HAVE_SMEM

vio/viopipe.c

@@ -141,5 +141,11 @@ int vio_close_pipe(Vio *vio)
   DBUG_RETURN(ret);
 }
 
+/* return number of bytes readable from pipe.*/
+uint vio_pending_pipe(Vio *vio)
+{
+  DWORD bytes;
+  return PeekNamedPipe(vio->hPipe, NULL, 0, NULL, &bytes, NULL) ? bytes : 0;
+}
 #endif
 

vio/viosocket.c

@@ -1214,7 +1214,6 @@ my_bool vio_is_connected(Vio *vio)
   DBUG_RETURN(bytes ? TRUE : FALSE);
 }
 
-#ifndef DBUG_OFF
 
 /**
   Number of bytes in the read or socket buffer
@@ -1233,22 +1232,34 @@ ssize_t vio_pending(Vio *vio)
     return vio->read_end - vio->read_pos;
 
   /* Skip non-socket based transport types. */
-  if (vio->type == VIO_TYPE_TCPIP || vio->type == VIO_TYPE_SOCKET)
+  switch (vio->type)
   {
+  case VIO_TYPE_TCPIP:
+    /* fallthrough */
+  case VIO_TYPE_SOCKET:
     /* Obtain number of readable bytes in the socket buffer. */
     if (socket_peek_read(vio, &bytes))
       return -1;
+    return bytes;
+
+  case VIO_TYPE_SSL:
+    bytes= (uint) SSL_pending(vio->ssl_arg);
+    if (bytes)
+      return bytes;
+    if (socket_peek_read(vio, &bytes))
+      return -1;
+    return bytes;
+
+#ifdef _WIN32
+  case VIO_TYPE_NAMEDPIPE:
+    bytes= vio_pending_pipe(vio);
+    return bytes;
+#endif
+  default:
+    return -1;
   }
-
-  /*
-    SSL not checked due to a yaSSL bug in SSL_pending that
-    causes it to attempt to read from the socket.
-  */
-
-  return (ssize_t) bytes;
 }
 
-#endif /* DBUG_OFF */
 
 /**
   Checks if the error code, returned by vio_getnameinfo(), means it was the