Fix qgis server fcgi response destructor

.ci/ogc/build.sh

@@ -2,7 +2,7 @@
 
 set -e
 
-mkdir /usr/src/qgis/build
+mkdir -p /usr/src/qgis/build
 cd /usr/src/qgis/build || exit 1
 
 export CCACHE_TEMPDIR=/tmp

scripts/cppcheck.sh

@@ -14,10 +14,12 @@ case $SCRIPT_DIR in
         ;;
 esac
 
+SRC_DIR=${1:-../src}
+
 LOG_FILE=/tmp/cppcheck_qgis.txt
 
 rm -f ${LOG_FILE}
-echo "Checking ${SCRIPT_DIR}/../src ..."
+echo "Checking ${SCRIPT_DIR}/${SRC_DIR} ..."
 
 # qgsgcptransformer.cpp causes an effective hang on newer cppcheck!
 
@@ -51,7 +53,7 @@ cppcheck --library=qt.cfg --inline-suppr \
          -DBUILTIN_UNREACHABLE="__builtin_unreachable();" \
          -i src/analysis/georeferencing/qgsgcptransformer.cpp \
          -j $(nproc) \
-         ${SCRIPT_DIR}/../src \
+         ${SCRIPT_DIR}/${SRC_DIR} \
          >>${LOG_FILE} 2>&1 &
 
 PID=$!

src/server/qgsfcgiserverresponse.cpp

@@ -27,8 +27,10 @@
 #include "qgslogger.h"
 
 #if defined( Q_OS_UNIX ) && !defined( Q_OS_ANDROID )
+#include <unistd.h>
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <chrono>
 
 //
 // QgsFCGXStreamData copied from libfcgi FCGX_Stream_Data
@@ -54,71 +56,128 @@ typedef struct QgsFCGXStreamData
 } QgsFCGXStreamData;
 #endif
 
-QgsSocketMonitoringThread::QgsSocketMonitoringThread( bool *isResponseFinished, QgsFeedback *feedback )
-  : mIsResponseFinished( isResponseFinished )
-  , mFeedback( feedback )
+// to be able to use 333ms expression as a duration
+using namespace std::chrono_literals;
+
+
+// QgsSocketMonitoringThread constructor
+QgsSocketMonitoringThread::QgsSocketMonitoringThread( std::shared_ptr<QgsFeedback> feedback )
+  : mFeedback( feedback )
   , mIpcFd( -1 )
 {
-  setObjectName( "FCGI socket monitor" );
-  Q_ASSERT( mIsResponseFinished );
   Q_ASSERT( mFeedback );
 
+  mShouldStop.store( false );
+
 #if defined( Q_OS_UNIX ) && !defined( Q_OS_ANDROID )
   if ( FCGI_stdout && FCGI_stdout->fcgx_stream && FCGI_stdout->fcgx_stream->data )
   {
-    QgsFCGXStreamData *stream = static_cast<QgsFCGXStreamData *>( FCGI_stdin->fcgx_stream->data );
+    QgsFCGXStreamData *stream = static_cast<QgsFCGXStreamData *>( FCGI_stdout->fcgx_stream->data );
     if ( stream && stream->reqDataPtr )
     {
       mIpcFd = stream->reqDataPtr->ipcFd;
     }
     else
     {
-      QgsMessageLog::logMessage( QStringLiteral( "FCGI_stdin stream data is null! Socket monitoring disable." ), QStringLiteral( "FCGIServer" ), Qgis::MessageLevel::Warning );
+      QgsMessageLog::logMessage( QStringLiteral( "FCGI_stdout stream data is null! Socket monitoring disabled." ), //
+                                 QStringLiteral( "FCGIServer" ),                                                   //
+                                 Qgis::MessageLevel::Warning );
     }
   }
   else
   {
-    QgsMessageLog::logMessage( QStringLiteral( "FCGI_stdin is null! Socket monitoring disable." ), QStringLiteral( "FCGIServer" ), Qgis::MessageLevel::Warning );
+    QgsMessageLog::logMessage( QStringLiteral( "FCGI_stdout is null! Socket monitoring disabled." ), //
+                               QStringLiteral( "FCGIServer" ),                                       //
+                               Qgis::MessageLevel::Warning );
   }
 #endif
 }
 
+// Informs the thread to quit
+void QgsSocketMonitoringThread::stop()
+{
+  mShouldStop.store( true );
+  // Release the mutex so the try_lock in the thread will not wait anymore and
+  // the thread will end its loop as we have set 'mShouldStop' to true
+  mMutex.unlock();
+}
+
 void QgsSocketMonitoringThread::run()
 {
+  // Lock the thread mutex: every try_lock will take 333ms
+  mMutex.lock();
+
   if ( mIpcFd < 0 )
   {
     QgsMessageLog::logMessage( QStringLiteral( "Socket monitoring disabled: no socket fd!" ), QStringLiteral( "FCGIServer" ), Qgis::MessageLevel::Warning );
     return;
   }
 
 #if defined( Q_OS_UNIX ) && !defined( Q_OS_ANDROID )
+  const pid_t threadId = gettid();
+
+  mShouldStop.store( false );
   char c;
-  while ( !*mIsResponseFinished )
+
+  fd_set setOptions;
+  FD_ZERO( &setOptions );        // clear the set
+  FD_SET( mIpcFd, &setOptions ); // add our file descriptor to the set
+
+  struct timeval timeout;
+  timeout.tv_sec = 0;
+  timeout.tv_usec = 50000; // max 50ms of timeout for select
+
+  while ( !mShouldStop.load() )
   {
-    const ssize_t x = recv( mIpcFd, &c, 1, MSG_PEEK | MSG_DONTWAIT ); // see https://stackoverflow.com/a/12402596
-    if ( x < 0 )
-    {
-      // Ie. we are still connected but we have an 'error' as there is nothing to read
-      QgsDebugMsgLevel( QStringLiteral( "FCGIServer: remote socket still connected. errno: %1" ).arg( errno ), 5 );
-    }
-    else
+    int rv = select( mIpcFd + 1, &setOptions, NULL, NULL, &timeout ); // see https://stackoverflow.com/a/30395738
+    if ( rv == -1 )
     {
       // socket closed, nothing can be read
-      QgsDebugMsgLevel( QStringLiteral( "FCGIServer: remote socket has been closed! errno: %1" ).arg( errno ), 2 );
+      QgsMessageLog::logMessage( QStringLiteral( "FCGIServer %1: remote socket has been closed (select)! errno: %2" ) //
+                                   .arg( threadId )
+                                   .arg( errno ),
+                                 QStringLiteral( "FCGIServer" ), Qgis::MessageLevel::Warning );
       mFeedback->cancel();
       break;
     }
+    else
+    {
+      const ssize_t x = recv( mIpcFd, &c, 1, MSG_PEEK | MSG_DONTWAIT ); // see https://stackoverflow.com/a/12402596
+      if ( x != 0 )
+      {
+        // Ie. we are still connected but we have an 'error' as there is nothing to read
+        QgsDebugMsgLevel( QStringLiteral( "FCGIServer %1: remote socket still connected. errno: %2, x: %3" ) //
+                            .arg( threadId )
+                            .arg( errno )
+                            .arg( x ),
+                          5 );
+      }
+      else
+      {
+        // socket closed, nothing can be read
+        QgsMessageLog::logMessage( QStringLiteral( "FCGIServer %1: remote socket has been closed (recv)! errno: %2, x: %3" ) //
+                                     .arg( threadId )
+                                     .arg( errno )
+                                     .arg( x ),
+                                   QStringLiteral( "FCGIServer" ), Qgis::MessageLevel::Warning );
+        mFeedback->cancel();
+        break;
+      }
+    }
 
-    QThread::msleep( 333L );
+    // If lock is acquired this means the response has finished and we will exit the while loop
+    // else we will wait max for 333ms.
+    if ( mMutex.try_lock_for( 333ms ) )
+      mMutex.unlock();
   }
 
-  if ( *mIsResponseFinished )
+  if ( mShouldStop.load() )
   {
-    QgsDebugMsgLevel( QStringLiteral( "FCGIServer: socket monitoring quits normally." ), 2 );
+    QgsDebugMsgLevel( QStringLiteral( "FCGIServer::run %1: socket monitoring quits normally." ).arg( threadId ), 2 );
   }
   else
   {
-    QgsDebugMsgLevel( QStringLiteral( "FCGIServer: socket monitoring quits: no more socket." ), 2 );
+    QgsDebugMsgLevel( QStringLiteral( "FCGIServer::run %1: socket monitoring quits: no more socket." ).arg( threadId ), 1 );
   }
 #endif
 }
@@ -134,15 +193,21 @@ QgsFcgiServerResponse::QgsFcgiServerResponse( QgsServerRequest::Method method )
   mBuffer.open( QIODevice::ReadWrite );
   setDefaultHeaders();
 
-  mSocketMonitoringThread = std::make_unique<QgsSocketMonitoringThread>( &mFinished, mFeedback.get() );
-  mSocketMonitoringThread->start();
+  mSocketMonitoringThread = std::make_unique<QgsSocketMonitoringThread>( mFeedback );
+
+  // Start the monitoring thread
+  mThread = std::thread( &QgsSocketMonitoringThread::run, mSocketMonitoringThread.get() );
 }
 
 QgsFcgiServerResponse::~QgsFcgiServerResponse()
 {
   mFinished = true;
-  mSocketMonitoringThread->exit();
-  mSocketMonitoringThread->wait();
+
+  // Inform the thread to quit asap
+  mSocketMonitoringThread->stop();
+
+  // Just to be sure
+  mThread.join();
 }
 
 void QgsFcgiServerResponse::removeHeader( const QString &key )
@@ -285,5 +350,5 @@ void QgsFcgiServerResponse::truncate()
 
 void QgsFcgiServerResponse::setDefaultHeaders()
 {
-  setHeader( QStringLiteral( "Server" ), QStringLiteral( " QGIS FCGI server - QGIS version %1" ).arg( Qgis::version() ) );
+  mHeaders.insert( QStringLiteral( "Server" ), QStringLiteral( " QGIS FCGI server - QGIS version %1" ).arg( Qgis::version() ) );
 }

src/server/qgsfcgiserverresponse.h

@@ -26,31 +26,41 @@
 #include "qgsserverresponse.h"
 
 #include <QBuffer>
-#include <QThread>
+#include <thread>
+#include <mutex>
 
 /**
  * \ingroup server
  * \class QgsSocketMonitoringThread
  * \brief Thread used to monitor the fcgi socket
  * \since QGIS 3.36
  */
-class QgsSocketMonitoringThread : public QThread
+class QgsSocketMonitoringThread
 {
-    Q_OBJECT
-
   public:
     /**
-     * \brief QgsSocketMonitoringThread
-     * \param  isResponseFinished
-     * \param  feedback
+     * Constructor for QgsSocketMonitoringThread
+     * \param  feedback used to cancel rendering jobs when socket timedout
+     */
+    QgsSocketMonitoringThread( std::shared_ptr<QgsFeedback> feedback );
+
+    /**
+     * main thread function
      */
-    QgsSocketMonitoringThread( bool *isResponseFinished, QgsFeedback *feedback );
     void run();
 
+    /**
+     * Stop the thread
+     */
+    void stop();
+
   private:
-    bool *mIsResponseFinished = nullptr;
-    QgsFeedback *mFeedback = nullptr;
+    std::atomic_bool mShouldStop;
+    std::shared_ptr<QgsFeedback> mFeedback;
     int mIpcFd = -1;
+
+    // used to synchronize socket monitoring thread and fcgi response
+    std::timed_mutex mMutex;
 };
 
 /**
@@ -66,7 +76,8 @@ class SERVER_EXPORT QgsFcgiServerResponse : public QgsServerResponse
      * \param method The HTTP method (Get by default)
      */
     QgsFcgiServerResponse( QgsServerRequest::Method method = QgsServerRequest::GetMethod );
-    virtual ~QgsFcgiServerResponse();
+
+    virtual ~QgsFcgiServerResponse() override;
 
     void setHeader( const QString &key, const QString &value ) override;
 
@@ -115,8 +126,12 @@ class SERVER_EXPORT QgsFcgiServerResponse : public QgsServerResponse
     QgsServerRequest::Method mMethod;
     int mStatusCode = 0;
 
+    // encapsulate thread data
     std::unique_ptr<QgsSocketMonitoringThread> mSocketMonitoringThread;
-    std::unique_ptr<QgsFeedback> mFeedback;
+    // real thread object. Used to join.
+    std::thread mThread;
+    // Used to cancel rendering jobs
+    std::shared_ptr<QgsFeedback> mFeedback;
 };
 
 #endif