Project

General

Profile

Actions

Bug #544

closed

a serious problem i found in WebControl.C, about multithread support

Added by DQ Qin over 13 years ago. Updated over 13 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
-
Start date:
09/29/2010
Due date:
% Done:

0%

Estimated time:

Description

refresh Wt application in browser(or reenter the http address in address bar) will cause server side crash.

i think i'v found the reason:

learn this code fragment in WebControl.C

void WebController::handleAsyncRequest(WebRequest *request)
{
  ...
#ifdef WT_THREADED
  boost::recursive_mutex::scoped_lock sessionsLock(mutex_);
#endif
  ...

  bool sessionDead = false;
  {
    WebSession::Handler handler(*session, *request, *(WebResponse *)request);

#ifdef WT_THREADED
    sessionsLock.unlock();
#endif

    session->handleRequest(handler);
    sessionDead = handler.sessionDead();
  }                                         //in Handler::~Handler() session will be delete 

  ...                                       //state without lock 
                                            //DANGEROUS MOMENT !!!!!!!!!!!!!!!!!!!!!!!!!!!
                                            //the deleted session will be accessed in other thread

  if (sessionDead)                          
    removeSession(sessionId);               //lock/unlock mutex_

  if (!running_)
    expireSessions();                       //lock/unlock mutex_

  ...
} 

in multi-thread enviroment

other thread will enter funciton expireSessions()

and access the deleted session that still in the SessionMap

when this thread unlock the mutex_

the crash happened

i'v test some examples with buildin http under windows,

the incidence rate is rather high

i try to change the code:

void WebController::handleAsyncRequest(WebRequest *request)
{
  ...
#ifdef WT_THREADED
  boost::recursive_mutex::scoped_lock sessionsLock(mutex_);
#endif
  ...

  bool sessionDead = false;
  {
    WebSession::Handler handler(*session, *request, *(WebResponse *)request);

    session->handleRequest(handler);
    sessionDead = handler.sessionDead();
  }                                         

  if (sessionDead)                          
    removeSession_WITHOUT_LOCK(sessionId);               

#ifdef WT_THREADED
    sessionsLock.unlock();
#endif

  if (!running_)
    expireSessions();                       //lock/unlock mutex_

  ...
} 


//add low level version of removeSession
void WebController::removeSession_WITHOUT_LOCK(const std::string& sessionId)
{
  SessionMap::iterator i = sessions_.find(sessionId);
  if (i != sessions_.end())
    sessions_.erase(i);
}

void WebController::removeSession(const std::string& sessionId)
{
#ifdef WT_THREADED
  boost::recursive_mutex::scoped_lock sessionsLock(mutex_);
#endif // WT_THREADED

  removeSession_WITHOUT_LOCK(sessionId);
}

main idea is unlock mutex_ AFTER the deleted session removed from sessionMap.

after i change the code, the crash never happened

but i don't know whether the problem is solved completely.

i wait your real solution

Actions #1

Updated by Koen Deforche over 13 years ago

  • Status changed from New to InProgress

Hey DQ Qin,

Well observed!

I guess this is problem is surfacing because we are now deleting a session actively when a page is unloaded. Your solution however does not allow concurrent handling of requests by different threads, so we will need to figure out another solution.

Regards,

koen

Actions #2

Updated by DQ Qin over 13 years ago

yes, my solution block the concurrent handling.

only for my private interest

I try another way:

void WebController::handleAsyncRequest(WebRequest *request)
{
  ...
#ifdef WT_THREADED
  boost::recursive_mutex::scoped_lock sessionsLock(mutex_);
#endif
  ...

  bool sessionDead = false;
  {
    WebSession::Handler handler(*session, *request, *(WebResponse *)request);

#ifdef WT_THREADED
    sessionsLock.unlock();
#endif

    session->handleRequest(handler);
    sessionDead = handler.sessionDead();

    if (sessionDead)                          
      removeSession(sessionId);             
  }                                         //in Handler::~Handler() session will be delete 
                                            //AFTER the session remove from sessions_ map

  if (!running_)
    expireSessions();                       

  ...
} 

only change the "}" position, for ~Handler() work later.

Actions #3

Updated by Koen Deforche over 13 years ago

Hey,

That solution is tempting but has the risk of a dead-lock: dead-lock avoidance dictates that you should always take multiple locks in the same order. Here you do not do this: you take session lock while sessions lock, and sessions lock while session lock.

I've come up with a good solution, which we'll commit to public git shortly.

Regards,

koen

Actions #4

Updated by Koen Deforche over 13 years ago

  • Status changed from InProgress to Resolved

Sometimes you find that fixing things simplifies everything in such a way that you wonder why you didn't figure that out in the first place...

Actions #5

Updated by DQ Qin over 13 years ago

thanks the real solution :-)

Actions #6

Updated by Koen Deforche over 13 years ago

  • Status changed from Resolved to Closed

Fixed in 3.1.6

Actions

Also available in: Atom PDF