Wt

Hey,

I follow your analysis and indeed this will lead to a memory leak.

IMO, your solution will not work because the shared pointer makes sure that reply stays alive as long as the http request object is passed around within Wt (this is until a flush(done) is called on it).

I believe the proper fix is to delete the httpRequest on Error, since in any case, consumeData() will be called with an Error state when the browser closes the connection.

Regards,

koen

Hi,

IMO, your solution will not work because the shared pointer makes sure that reply stays alive as long as the http request object is passed around within Wt (this is until a flush(done) is called on it).

Do you mean that HTTPRequest that is stored inside WtReply can outlive WtReply itself ? Could you show me the exact line of code where that happens ?

Ok, but about:

both could be deleted while the http request is still referenced and used within Wt

If we look at WtReply.C, there're only 2 places where 'httpRequest_' private member gets out of WtReply:

    if (!connection->server()->controller()->requestDataReceived
        (httpRequest_, bodyReceived_, request().contentLength)) {
      delete httpRequest_;
      httpRequest_ = 0;

      setStatus(request_entity_too_large);
      setCloseConnection();
      state = Request::Error;
    }

and

    connection->server()->controller()->handleRequest(httpRequest_);

Now, let's look at first method - requestDataReceived:

bool WebController::requestDataReceived(WebRequest *request,
                    boost::uintmax_t current,
                    boost::uintmax_t total)
{
#ifdef WT_THREADED
  boost::mutex::scoped_lock lock(uploadProgressUrlsMutex_);
#endif // WT_THREADED

  if (uploadProgressUrls_.find(request->queryString())
      != uploadProgressUrls_.end()) {
#ifdef WT_THREADED
    lock.unlock();
#endif // WT_THREADED

    CgiParser cgi(conf_.maxRequestSize());

    try {
      cgi.parse(*request, CgiParser::ReadHeadersOnly);
    } catch (std::exception& e) {
      LOG_ERROR_S(&server_, "could not parse request: " << e.what());
      return false;
    }

    const std::string *wtdE = request->getParameter("wtd");
    if (!wtdE)
      return false;

    std::string sessionId = *wtdE;

    ApplicationEvent event(sessionId,
               boost::bind(&WebController::updateResourceProgress,
                       this, request, current, total));

    if (handleApplicationEvent(event))
      return !request->postDataExceeded();
    else
      return false;
  }

  return true;
}

and second - handleRequest:

void WebController::handleRequest(WebRequest *request)
{
... too long to quote ...
}

So, it looks like 'httpRequest*' is only being used "in place", it doesn't get stored anywhere, so, after those methods return we can safely delete 'httpRequest*'. Or am I missing something ? Sorry if I'm being annoying, I just want to understand why it's not possible to just make a raw pointer, I'm using Wt in my embedded project and I can't wait until this bug is officially fixed, it causes a lot of problems especially with large file uploads, due to this bug spool files remain in filesystem when they should have been deleted, thus, my embedded system can run low on disk space quickly...

Hey,

I didn't want to mark the bug as resolved but latest git does resolve it. I want to fix it better by propagating the close event.

For asynchronous responses, the request is kept around (in WApplication and WResource continuation).

Regards,

Koen