Re: Problem interrupting threads in PR_Accept() wiht PR_Interrupt()

On Wed, Aug 6, 2008 at 7:38 AM, Joachim Ziegler <[email protected]> wrote:
> Hello,
>
> to get familiarized with NSPR, I have written a little demo web server.
>
> The server has master thread that creates 5 worker threads. Each worker
> thread blocks in PR_Accept() to wait for a new request, then serves the
> request, the goes on in an endless loop. This loop looks like this:
>
>
>  while( 1 ) {
>    if((fd = PR_Accept( sock, NULL, PR_INTERVAL_NO_TIMEOUT ))
>       == NULL) {
>      if(PR_GetError() == PR_PENDING_INTERRUPT_ERROR)
>        break; /* that's OK: master has signaled an interrupt */
>      else {
>        oops("PR_Accept in thread_start()");
>        continue;
>      }
>    }
>    handle_call( fd );
>    PR_Close( fd );
>  }
>
>  /* this line should be reached on a clean shutdown*/
>  printf("\t[%d] Thread %d is shutting down.\n",
>         myPID, nthread);
>
>
> Now I want to signal the worker threads to properly shut down, that is,
> to break out of the loop. I do this by making the master thread issue
>
> for(nthread = 0; nthread < NTHREADS; nthread++)
>  if( PR_Interrupt(worker[nthread]) == PR_SUCCESS )
>    printf("MASTER[%d]: Thread %d is blocked. Sent PR_Interrupt.\n",
> myPID, nthread);
>  else
>    printf("MASTER[%d]: Thread %d is not blocked. Sent PR_Interrupt\n",
> myPID, nthread);

Your code is correct.  There is only a misunderstanding in the printf
messages for PR_Interrupt.  PR_Interrupt returns PR_SUCCESS
not because the target thread is blocked, but because the target
thread is valid.  In other words, the only reason PR_Interrupt
returns PR_FAILURE is that the target thread is invalid, for
example, a NULL PRThread* pointer.

What platform are you testing this code on?  We have different
implementations for Unix and Windows.

Could you open a bug report for this problem?

If you are comfortable debugging NSPR, here are the relevant
places to look at, assuming you're testing your code on Unix.

1. This is where PR_Interrupt sets the "interrupted" flag in the
target thread:
http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/pthreads/ptthread.c#742

740     if (NULL == thred) return PR_FAILURE;
741
742     thred->state |= PT_THREAD_ABORTED;  <=== SETSFLAG
743
744     cv = thred->waiting;

Note that it is done without the protection of a lock.  This could be
problematic.

2. This is where PR_Accept checks the "interrupted" flag before
calling accept() and blocking, and returns NULL if it is set:
http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/pthreads/ptio.c#1660

1656     PRFileDesc *newfd = NULL;
1657     PRIntn syserrno, osfd = -1;
1658     pt_SockLen addr_len = sizeof(PRNetAddr);
1659
1660     if (pt_TestAbort()) return newfd;  <=== CHECKS FLAG

3. This is where PR_Accept checks the "interrupted" flag when
it is blocking:
http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/pthreads/ptio.c#601

599                 rv = poll(&tmp_pfd, 1, msecs);
600
601                 if (self->state & PT_THREAD_ABORTED)   <=== CHECKS FLAG
602                 {
603                     self->state &= ~PT_THREAD_ABORTED;
604                     op->result.code = -1;
605                     op->syserrno = EINTR;
606                     op->status = pt_continuation_done;
607                     return;
608                 }

Note that it blocks in poll() for 5 seconds (PT_DEFAULT_POLL_MSEC=5000)
and checks the "interrupted" flag.  So PR_Accept() may take up to 5 seconds
to respond to PR_Interrupt.

So the only problems are:
- The interrupt may take up to 5 seconds to take effect.  This is a
known limitation.
- The setting and checking of the "interrupted" flag (PT_THREAD_ABORTED) is
  not thread-safe.  I can give you a patch that adds a lock.

Wan-Teh