fix(qgisserver): suppress wait in fcgi response destructor

In the previous version, the response destructor waited for the monitoring thread to end that may induce a 333ms sleep each time. We fix that by using a classic ptr to separate the thread and the response lifecycles.
qgis · Nov 28, 2024 · fc27b8d · fc27b8d
1 parent da86bc1
commit fc27b8d
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 15 deletions.
diff --git a/src/server/qgsfcgiserverresponse.cpp b/src/server/qgsfcgiserverresponse.cpp
@@ -54,13 +54,11 @@ typedef struct QgsFCGXStreamData
 } QgsFCGXStreamData;
 #endif
 
-QgsSocketMonitoringThread::QgsSocketMonitoringThread( bool *isResponseFinished, QgsFeedback *feedback )
-  : mIsResponseFinished( isResponseFinished )
-  , mFeedback( feedback )
+QgsSocketMonitoringThread::QgsSocketMonitoringThread( QgsFeedback *feedback )
+  : mFeedback( feedback )
   , mIpcFd( -1 )
 {
   setObjectName( "FCGI socket monitor" );
-  Q_ASSERT( mIsResponseFinished );
   Q_ASSERT( mFeedback );
 
 #if defined(Q_OS_UNIX) && !defined(Q_OS_ANDROID)
@@ -73,20 +71,29 @@ QgsSocketMonitoringThread::QgsSocketMonitoringThread( bool *isResponseFinished,
     }
     else
     {
-      QgsMessageLog::logMessage( QStringLiteral( "FCGI_stdin stream data is null! Socket monitoring disable." ),
+      QgsMessageLog::logMessage( QStringLiteral( "FCGI_stdin 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_stdin is null! Socket monitoring disabled." ),
                                QStringLiteral( "FCGIServer" ),
                                Qgis::MessageLevel::Warning );
   }
+
+  // Suicide the thread when it ends
+  connect( this, &QThread::finished, this, &QThread::deleteLater );
+
 #endif
 }
 
+void QgsSocketMonitoringThread::setResponseFinished( bool responseFinished )
+{
+  mIsResponseFinished = responseFinished;
+}
+
 void QgsSocketMonitoringThread::run( )
 {
   if ( mIpcFd < 0 )
@@ -99,7 +106,7 @@ void QgsSocketMonitoringThread::run( )
 
 #if defined(Q_OS_UNIX) && !defined(Q_OS_ANDROID)
   char c;
-  while ( !*mIsResponseFinished )
+  while ( !mIsResponseFinished )
   {
     const ssize_t x = recv( mIpcFd, &c, 1, MSG_PEEK | MSG_DONTWAIT ); // see https://stackoverflow.com/a/12402596
     if ( x < 0 )
@@ -118,7 +125,7 @@ void QgsSocketMonitoringThread::run( )
     QThread::msleep( 333L );
   }
 
-  if ( *mIsResponseFinished )
+  if ( mIsResponseFinished )
   {
     QgsDebugMsgLevel( QStringLiteral( "FCGIServer: socket monitoring quits normally." ), 2 );
   }
@@ -141,15 +148,39 @@ QgsFcgiServerResponse::QgsFcgiServerResponse( QgsServerRequest::Method method )
   mBuffer.open( QIODevice::ReadWrite );
   setDefaultHeaders();
 
-  mSocketMonitoringThread = std::make_unique<QgsSocketMonitoringThread>( &mFinished, mFeedback.get() );
+  // This is not a unique_ptr because we want the response to not depend on the thread lifecycle.
+  mSocketMonitoringThread = new QgsSocketMonitoringThread( mFeedback.get() );
   mSocketMonitoringThread->start();
 }
 
+QgsFcgiServerResponse::QgsFcgiServerResponse( const QgsFcgiServerResponse &copy )
+  : mFinished( copy.mFinished )
+  , mHeadersSent( copy.mHeadersSent )
+  , mMethod( copy.mMethod )
+  , mStatusCode( copy.mStatusCode )
+  , mSocketMonitoringThread( nullptr )
+  , mFeedback( nullptr )
+{
+}
+
+QgsFcgiServerResponse &QgsFcgiServerResponse::operator =( const QgsFcgiServerResponse &copy )
+{
+  mFinished = copy.mFinished;
+  mHeadersSent = copy.mHeadersSent;
+  mMethod = copy.mMethod;
+  mStatusCode = copy.mStatusCode;
+  mSocketMonitoringThread = nullptr;
+  mFeedback = nullptr;
+
+  return *this;
+}
+
 QgsFcgiServerResponse::~QgsFcgiServerResponse()
 {
   mFinished = true;
-  mSocketMonitoringThread->exit();
-  mSocketMonitoringThread->wait();
+  if ( mSocketMonitoringThread )
+    // This will allow the thread to finish sleeping and exit its while loop in the background, without us needing to wait for it to finish.
+    mSocketMonitoringThread->setResponseFinished( mFinished );
 }
 
 void QgsFcgiServerResponse::removeHeader( const QString &key )

diff --git a/src/server/qgsfcgiserverresponse.h b/src/server/qgsfcgiserverresponse.h
@@ -45,11 +45,13 @@ class QgsSocketMonitoringThread: public QThread
      * \param  isResponseFinished
      * \param  feedback
      */
-    QgsSocketMonitoringThread( bool *isResponseFinished, QgsFeedback *feedback );
+    QgsSocketMonitoringThread( QgsFeedback *feedback );
     void run( );
 
+    void setResponseFinished( bool responseFinished );
+
   private:
-    bool *mIsResponseFinished = nullptr;
+    bool mIsResponseFinished = false;
     QgsFeedback *mFeedback = nullptr;
     int mIpcFd = -1;
 };
@@ -68,7 +70,18 @@ class SERVER_EXPORT QgsFcgiServerResponse: public QgsServerResponse
      * \param method The HTTP method (Get by default)
      */
     QgsFcgiServerResponse( QgsServerRequest::Method method = QgsServerRequest::GetMethod );
-    virtual ~QgsFcgiServerResponse();
+
+    /**
+     *  Dummy copy constructor. Needed because of QgsSocketMonitoringThread ptr
+     */
+    QgsFcgiServerResponse( const QgsFcgiServerResponse &copy );
+
+    /**
+     * Dummy operator = . Needed because of QgsSocketMonitoringThread ptr
+     */
+    QgsFcgiServerResponse &operator = ( const QgsFcgiServerResponse &copy );
+
+    virtual ~QgsFcgiServerResponse() override;
 
     void setHeader( const QString &key, const QString &value ) override;
 
@@ -117,7 +130,7 @@ class SERVER_EXPORT QgsFcgiServerResponse: public QgsServerResponse
     QgsServerRequest::Method mMethod;
     int mStatusCode = 0;
 
-    std::unique_ptr<QgsSocketMonitoringThread> mSocketMonitoringThread;
+    QgsSocketMonitoringThread *mSocketMonitoringThread;
     std::unique_ptr<QgsFeedback> mFeedback;
 };