[EOS-CTA] Moves Request serialization from ServiceClientSide to Request

Necessary in order to make ServiceClientSide reentrant

xroot_ssi_pb/XrdSsiPbRequest.hpp

@@ -20,7 +20,6 @@
 #define __XRD_SSI_PB_REQUEST_H
 
 #include <future>
-
 #include <XrdSsi/XrdSsiRequest.hh>
 
 namespace XrdSsiPb {
@@ -50,24 +49,9 @@ template <typename RequestType, typename MetadataType, typename AlertType>
 class Request : public XrdSsiRequest
 {
 public:
-   Request(const std::string &buffer_str, unsigned int response_bufsize, uint16_t timeout) :
-      m_request_bufptr(buffer_str.c_str()),
-      m_request_bufsize(buffer_str.size()),
-      m_response_bufsize(response_bufsize)
-   {
-#ifdef XRDSSI_DEBUG
-      std::cout << "[DEBUG] Request constructor: "
-                << "Response buffer size = " << m_response_bufsize
-                << ", Response timeout = " << timeout << std::endl;
-#endif
-
-      // Set response timeout
-
-      SetTimeOut(timeout);
-   }
+   Request(const RequestType &request, unsigned int response_bufsize, uint16_t timeout);
 
-   virtual ~Request() 
-   {
+   virtual ~Request() {
 #ifdef XRDSSI_DEBUG
       std::cout << "[DEBUG] ~Request destructor" << std::endl;
 #endif
@@ -76,18 +60,18 @@ public:
    /*!
     * The implementation of GetRequest() must create request data, save it in some manner, and provide
     * it to the framework.
-    *
-    * Optionally also define the RelRequestBuffer() method to clean up when the framework no longer
-    * needs access to the data. The thread used to initiate a request may be the same one used in the
-    * GetRequest() call.
     */
 
    virtual char *GetRequest(int &reqlen) override
    {
-      reqlen = m_request_bufsize;
-      return const_cast<char*>(m_request_bufptr);
+      reqlen = m_request_str.size();
+      return const_cast<char*>(m_request_str.c_str());
    }
 
+   // Optionally also define the RelRequestBuffer() method to clean up when the framework no longer
+   // needs access to the data. The thread used to initiate a request may be the same one used in the
+   // GetRequest() call.
+
    virtual bool ProcessResponse(const XrdSsiErrInfo &eInfo, const XrdSsiRespInfo &rInfo) override;
 
    virtual XrdSsiRequest::PRD_Xeq ProcessResponseData(const XrdSsiErrInfo &eInfo, char *buff, int blen, bool last) override;
@@ -101,10 +85,9 @@ public:
    auto GetFuture() { return m_promise.get_future(); }
 
 private:
-   const char *m_request_bufptr;                //!< Pointer to the Request buffer
-   int         m_request_bufsize;               //!< Size of the Request buffer
-   char       *m_response_bufptr;               //!< Pointer to the Response buffer
-   int         m_response_bufsize;              //!< Size of the Response buffer
+   std::string  m_request_str;                  //!< Request buffer
+   char        *m_response_bufptr;              //!< Pointer to the Response buffer
+   int          m_response_bufsize;             //!< Size of the Response buffer
 
    std::promise<MetadataType> m_promise;        //!< Promise a reply of Metadata type
    RequestCallback<AlertType> AlertCallback;    //!< Callback for Alerts
@@ -112,6 +95,34 @@ private:
 
 
 
+/*!
+ * Request constructor
+ */
+
+template<typename RequestType, typename MetadataType, typename AlertType>
+Request<RequestType, MetadataType, AlertType>::
+Request(const RequestType &request, unsigned int response_bufsize, uint16_t timeout) :
+   m_response_bufsize(response_bufsize)
+{
+#ifdef XRDSSI_DEBUG
+   std::cout << "[DEBUG] Request constructor: "
+             << "Response buffer size = " << m_response_bufsize
+             << " bytes, timeout = " << timeout << std::endl;
+#endif
+   // Set response timeout
+
+   SetTimeOut(timeout);
+
+   // Serialize the Request
+
+   if(!request.SerializeToString(&m_request_str))
+   {
+      throw PbException("request.SerializeToString() failed");
+   }
+}
+
+
+
 /*!
  * Process Responses from the server
  *

xroot_ssi_pb/XrdSsiPbRequestProc.hpp

@@ -136,6 +136,8 @@ template <typename RequestType, typename MetadataType, typename AlertType>
 void RequestProc<RequestType, MetadataType, AlertType>::Execute()
 {
    const int ExecuteTimeout = 15;    //< Maximum no. of seconds to wait before deleting myself
+                                     //< What is a sensible number? Does it need to be configurable?
+                                     //< In any case it should be <= timeout on the client side?
 
 #ifdef XRDSSI_DEBUG
    std::cout << "[DEBUG] RequestProc::Execute()" << std::endl;
@@ -199,7 +201,9 @@ void RequestProc<RequestType, MetadataType, AlertType>::Execute()
    {
       throw XrdSsiException("RequestProc::Finished() was never called!");
 
-      // Potentially could call Finished() with cancel == true here instead of throwing an exception?
+      // Should call Finished(true) instead of throwing an exception, waiting for Andy to comment on
+      // whether the handling of Finished() is reentrant in the framework, or whether the application
+      // has to manage it.
    }
 }
 

xroot_ssi_pb/XrdSsiPbServiceClientSide.hpp

@@ -70,8 +70,6 @@ private:
 
    unsigned int   m_response_bufsize;    //!< Buffer size for responses from the XRootD SSI server
    unsigned int   m_server_tmo;          //!< Timeout for a response from the server
-
-   std::string    m_request_str;         //!< Buffer for sending a request
 };
 
 
@@ -153,26 +151,21 @@ ServiceClientSide<RequestType, MetadataType, AlertType>::~ServiceClientSide()
 template <typename RequestType, typename MetadataType, typename AlertType>
 MetadataType ServiceClientSide<RequestType, MetadataType, AlertType>::Send(const RequestType &request)
 {
-   // Serialize the Request
-
-   if(!request.SerializeToString(&m_request_str))
-   {
-      throw PbException("request.SerializeToString() failed");
-   }
-
    // Instantiate the Request object
 
-   auto request_ptr = new Request<RequestType, MetadataType, AlertType>(m_request_str, m_response_bufsize, m_server_tmo);
+   auto request_ptr = new Request<RequestType, MetadataType, AlertType>(request, m_response_bufsize, m_server_tmo);
 
    auto future_response = request_ptr->GetFuture();
 
-   // Transfer ownership of the Request to the Service object. The framework will handle deletion of the
-   // Request object. Although it is safe to delete the Resource after ProcessRequest() returns, we are
-   // creating reusable Resources, so no need to delete it.
+   // Transfer ownership of the Request to the Service object.
 
    m_server_ptr->ProcessRequest(*request_ptr, m_resource);
 
-   // Wait synchronously for the framework to return its response
+   // After ProcessRequest() returns, it is safe for request_ptr to go out-of-scope, as the framework
+   // will handle deletion of the Request object. It is also safe to delete the Resource; in our case
+   // we do not need to as we created a reusable Resource.
+
+   // Wait synchronously for the framework to return its Response (or an exception)
 
    auto response = future_response.get();