Fix/fcgi response destructor

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,41 +56,57 @@ 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." ),
+      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." ),
+    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!" ),
@@ -98,33 +116,59 @@ void QgsSocketMonitoringThread::run( )
   }
 
 #if defined(Q_OS_UNIX) && !defined(Q_OS_ANDROID)
+#ifdef QGISDEBUG
+  const pid_t threadId = gettid();
+#endif
+
+  mShouldStop.store( false );
   char c;
-  while ( !*mIsResponseFinished )
+  while ( !mShouldStop.load() )
   {
     const ssize_t x = recv( mIpcFd, &c, 1, MSG_PEEK | MSG_DONTWAIT ); // see https://stackoverflow.com/a/12402596
-    if ( x < 0 )
+    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 );
+      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
-      QgsDebugMsgLevel( QStringLiteral( "FCGIServer: remote socket has been closed! errno: %1" ).arg( errno ), 2 );
-      mFeedback->cancel();
-      break;
+      // double check...
+      ssize_t x2 = 0;
+      for ( int i = 0; !mShouldStop.load() && x2 == 0 && i < 10; i++ )
+      {
+        x2 = recv( mIpcFd, &c, 1, MSG_PEEK | MSG_DONTWAIT ); // see https://stackoverflow.com/a/12402596
+        std::this_thread::sleep_for( 50ms );
+      }
+      if ( x2 == 0 )
+      {
+        // socket closed, nothing can be read
+        QgsDebugMsgLevel( QStringLiteral( "FCGIServer %1: remote socket has been closed! errno: %2, x: %3" )
+                          .arg( threadId ).arg( errno ).arg( x2 ), 2 );
+        mFeedback->cancel();
+        break;
+      }
+      else
+      {
+        QgsDebugMsgLevel( QStringLiteral( "FCGIServer::run %1: remote socket is not closed in fact! errno: %2, x: %3" )
+                          .arg( threadId ).arg( errno ).arg( x2 ), 1 );
+      }
+
     }
 
-    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
 }
@@ -141,15 +185,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 )
@@ -296,5 +346,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,32 +26,42 @@
 #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( bool *isResponseFinished, QgsFeedback *feedback );
-    void run( );
+    QgsSocketMonitoringThread( std::shared_ptr<QgsFeedback> feedback );
+
+    /**
+     * main thread function
+     */
+    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;
 };
 
 /**
@@ -68,7 +78,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;
 
@@ -117,8 +128,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