Re: output.c error in multithreaded program

From: Joseph M. Newcomer (newcomer_at_flounder.com)
Date: 12/27/04 

Date: Mon, 27 Dec 2004 02:52:55 -0500

CWinThread::Create is essentially AfxBeginThread. Rarely is it necessary to create an
instance of CWinThread and start it separately.

pThread = (CThreadType1 *)AfxBeginThread(CThreadType1::ThreadFunc, ThreadParams);

usually suffices. You don't need to store the thread ID because you can always get it if
you need it just by calling ::GetCurrentThreadId() and in any case it makes no sense to
ask for the pThread->m_n_ThreadID because at the point where you ask for it, pThread may
no longer exist as a valid pointer. In fact, you cannot access pThread for any reason from
any other thread after you start the thread running (this is a fundamental synchronization
error) unless you have set the m_bAutoDelete flag to FALSE, which I don't see happening
here. But since there is no need to access it, just delete that line. If you need the
thread ID, just ask for it. as needed (unless you need it tens of thousands of times per
second, don't worry about silly ideas like "efficiency" in this context).

But if you have synchronization errors like this, you probably have lots of others.
Threading is nontrivial, since most styles don't work terribly well with it. No global
variables, no static variables, assume actual concurrency. Never SendMessage. Never touch
a writeable variable that is not local to the thread without synchronization. Be very
careful about passing address of local variables; never pass one outside the thread
context, for example. (There's an exception, but it is very, very delicate to do, and
highly unusual, and fairly exotic).

More below...

On Mon, 27 Dec 2004 02:04:49 -0000, "Joe" <not@home.com> wrote:

>
>"Joseph M. Newcomer" <newcomer@flounder.com> wrote in message
>news:lq8us0146dtir0o1cc7g3vc4l1rf3iqdpv@4ax.com...
>> See below...
>>
>> You have not indicated how you create the threads; I hope it is
>AfxBeginThread. If it is
>> anything else, you are doomed, so fix it if so.
>
>I'm doomed DOOMED D O O M E D!!!!! ;-)
>
>I'm drilling a website and I used a very old demo here as my model.
>http://www.codeguru.com/Cpp/I-N/internet/generalinternet/article.php/c3413
>
>
>Each thread drills a page (on the same website) and I have set up
>dependencies between threads (I'm not sure I'm doing that right either -
>please see further below about a couple of semaphore questions).
>
>So I do (snippets):
>
>class CThreadType1: public CWinThread
>
>then:
>
>pThread = new CThreadType1(CThreadType1::ThreadFunc,pThreadParams);
>if(startThread(pThread)==FALSE)
>{
>//deletes
>}
>pThreadParams->m_threadID = pThread->m_nThreadID;
>
>where:
>
>BOOL SimpleClassFactory::startThread (CSpiderThread *pThread)
>{
>if (pThread == NULL)
>{
>AfxMessageBox("Cannot Start New Thread");
>return FALSE;
>}
>
>// Starts execution of a CWinThread object
>if (!pThread->CreateThread())
>{
>AfxMessageBox("Cannot Start New Thread");
>return FALSE;
>}
>return TRUE;
>}
>
>
>
>
>>
>> It is not clear that you need multithreaded locking; you have not
>adequately defined the
>> problem.
>
>Actually for the logging function I don't *think* I do, I was clutching at
>straws.
>The loggng function is in the parent process and the child threads use
>sendmessage to access it . I assumed this is implicitely thread safe.
>
>All you say is that it "crashes" (whatever THAT means!) in
>output.c, but you
>> don't say where, or in what routine; instead, you merely state a comment
>from the file
>> itself. This doesn't really help us much. "My program crashs somewhere" is
>pretty vague,
>> and knowing the "somewhere" is output.c isn't a lot of help, given that it
>encompasses
>> several functions and contains 1350 source lines. You don't even say which
>version of VS
>> you are using!
>
>OK sorry. I see your point, but in my defense I never know how specific to
>get in case it requires the reader to put too much effort in to help. And I
>could always be doing something particularly dumb where a general reply
>might see me straight.
>
>
>> If you look at the call stack, it should tell you what the functions are
>on the stack, and
>> what their arguments are. I strongly suspect that the file argument to
>whatever function
>> you are calling is 00000000, meaning you failed to check the return code
>from fopen, which
>> failed.
>>
>
>I did say in my original post I had mostly ntdll.dll which I can't
>decipher - from memory there was output.c and one other windows function (ie
>not one of my application functions). I'm afraid I didn't record anymore
>details - I will do next time ;)
>
>~~~~~~~~~~~~~~~~~~~~
>
>
>So I take it there are problems with CreateThread() and I should substitute
>AfxBeginThread. But I've had a quick
>scan of your article (I wanted to get this post sent before bed)
>
>http://www.codeproject.com/threads/UsingUIThreads.asp
>
>at it uses CreateThread() - so I'm a little confused.
>
>
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
>Here's a second part to my question. I have two issues relating to
>semaphores:
>
>Firstly.
>#####
>
>I want to set up priorites for the
>threads to access the internet - but I could find any examples so I set up
>this structure using semaphores. I include the function desc which should
>explain my rationale
>
>dwRet=GetHttpFileHarness(LOW_PRIORITY,pThrPars->m_strServerName,pThrPars->m_
>strObject, pThrPars,FALSE);
****
I don't know what you mean by "priorities". Do you mean that requests will not be in FIFO
order? That's easy: just modify your queue insertion routine to put the request in the
queue in priority order. This still requires a semaphore to notify the service thread(s)
that there is something in the queue, and it requires a CRITICAL_SECTION to lock the queue
while it is being manipulated. It is almost always a Very Bad Idea to play games with
thread scheduling priorities. Mostly because you will never get it right (if you don't
know what the Balance Set Manager is, for example, you are in trouble if you think
scheduling priorities give you control over priority of how things are done).

Look at the symmetric producer/consumer model. You just get most of this free, but there
is a serious problem in any of these models with something called "priority inversion",
where all the threads are busy handling low-priority tasks and no thread is available to
handle high-priority work. In this case, multiple queues are often a solution, with server
threads attached to each queue.
*****
>
>
>
>/*
>Right I've not found a way to ensure that, across processes, we can manage
>priorities such that a new request with a higher priority will push in from
>of a
>previously waiting thread with a lower priority.
>So a cheap-and-cheerful way of ensuring that "some" of the higher priority
>requests will always be being processed is to have a secondary Semaphore
>which limits the
>max number of lower priority request to be fewer than the total allowed. The
>difference
>will always be available to the higher priority.
*****
Multiple queues are probably the best solution
*****
>*/
>
>DWORD CThreadHTTP::GetHttpFileHarness(UINT iPriority,LPCTSTR
>ServerName,LPCTSTR strObject,CThrPars *pThreadParams,BOOL ViewFile)
>{ //(LPARAM)line
>DWORD dwRet = FALSE;
>long lcurcount=0;
>long lcurcount2=0;
>
>if(iPriority==LOW_PRIORITY)
>{
>WaitForSingleObject(hSemGMAXPLOWhttp,INFINITE);
>}
>
>WaitForSingleObject(hSemGMAXhttp,INFINITE);//decrements the semaphore
>dwRet=GetHttpFile(ServerName,strObject,pThreadParams,ViewFile);
>ReleaseSemaphore(hSemGMAXhttp,1,&lcurcount);
>
>if(iPriority==LOW_PRIORITY)
>{
>ReleaseSemaphore(hSemGMAXPLOWhttp,1,&lcurcount2);
>}
>return(dwRet);
>}
>
>GetHttpFile and all the other internet stuff is basically a cut and paste
>from the demo. I think I am getting problems when the site does not reply or
>a thread has to wait too long it seems to hang. In such a case I *think* I
>am blocking other threads from internet access as the semaphores are not
>getting released. This problem then cascades and I get lockout of the low
>priority threads ( I think the type of webpages I give a priority of type
>LOW return a lot of info and are prone to delay).
*****
Generally, it is best to use asynchronous communication; I know there's a set of
asynchronous HTTP classes lurking somewhere, but I've not used them. Otherwise, you get
into serious problems of indefinite resource consumption. In this case, a given thread
could have several outstanding connections, which is quite feasilble if you use UI threads
(AfxBeginThread(RUNTIME_CLASS(CMyThreadClass), params) will do the job).
*****
>
>At time of spotting your reply I was just about to investigate this -
>however I have limited experience with "internet programming" (well just
>this program - so I'm not sure how I can adequately test this - particularly
>in this multithreaded environment!).
>
>Have you (anyone) any observations?
>
>
>Secondly
>#######
>
>As I said I have dependencies betwen the threads.
>
>For example. I have one thread A which downloads a "header page" from the
>site kicks off B with a list of pages to
>return. A waits. B kicks off a number of threads (type C ( max ~40 )) to
>return the corresponding "detail" pages. B limits the maximum number of
>concurrent type C's running
>( I do this as one process may have many groups of ABC) and I have several
>processes. This is also why I have this global semaphore above as I don't
>wish to overload the site for obvious reasons).
*****
All you have here is a set of servers which you trigger by putting requests in their
queues. There would be no need for a global semaphore under any conditions; one semaphore
per queue would do nicely. With asynchronous communications you don't need a lot of
threads, so look into that.
*****
>
>So A waits until all the pages have been successfully returned ( all C's
>completed) as it must process all pages together - remember I can/will have
>several groups ABC in a process so I can have a lot going on at the same
>time.
*****
Making A wait is probably a Bad Idea. Instead, have a way of determining when all pages
have completed and then fire off an event that causes them to be processed. You're still
caught up in thinking in a synchronous model, which is rarely a good idea. It leads to
confusing threading, which is what you have here, and unnecessarily complex interthread
signalling. Given B has the list of pages, the number is known, so there is no need to
wait at all. Just let A build the requests and go away, or at least just wait for a need
to generate more requests. There are a lot of interesting fully-asynchronous structures
that are a lot easier to deal with under situations like this.
*****
>
>Anyway, here is the structure I have dreamed up to implement this dependency
>waiting, does it look OK?
>
>class CEventState is a heap object avaliable to all threads in a group ABC
>through a passed pointer in the input thread params.
>
>
>CEventState
>
>BOOL createBlockingEventSemaphore(int iSem, long lTotal, long lBlocked)
>{
>::EnterCriticalSection(&lock);
>handle[iSem] = CreateSemaphore( NULL,0,lBlocked,NULL);
>lSemCount[iSem] = lTotal;//block until this ct decrements to zero
>::LeaveCriticalSection(&lock);
>return(true);
>}
*****
Why enter a critical section before creating a semaphore? Why create an array of
semaphores? Why have an array of handles and an array of counts when the natural
representation would be an array of struct/class objects that had a handle and count
member, but there is no reason I can think of to have an array at all!
*****
>
>
>void maintainBlockingSemaphores(int iAmt)
>{
>::EnterCriticalSection(&lock);
>lSemCount[SEM_BLOCKING]+=iAmt; //negative iAmt decreases the blocking count
>if(lSemCount[SEM_BLOCKING]==0) //all done, so release semaphore lock that
>thread(s) are waiting on
>{ ReleaseSemaphore( handle[SEM_BLOCKING],1,NULL);
>::LeaveCriticalSection(&lock);
>}
*****
All previous comments apply here. Something is Very Wrong with this picture. The whole
notion of an array is deeply suspect.
*****
>
>
>Type A creates B then waits on
>
>WaitForSingleObject(pEvent->handle[SEM_BLOCKING],INFINITE);//waits until all
>type C threads terminated
>
*****
Why not allocate an object like this:

class Waiter {
     public:
          Waiter(long n) { count = n; event = ::CreateEvent(NULL, TRUE, FALSE, NULL); }
          // check parameters: it should be manual-reset, initial non-signalled
          void Completed() {
                         if(InterlockedDecrement(&count) == 0)
                                ::SetEvent(event);
                         }
         void Wait() { ::WaitForSingleObject(event, INFINITE); }
    protected:
          long count;
          HANDLE event;
}

Create a new one of these objects when you launch a thread, e.g.,

Waiter * w = new Waiter;

then you could follow your model and simply pass a pointer to this into each thread that
is collecting a page. As each thread collects a page, it calls the Completed() method.
Meahwnile, the thread A has done a Wait() call on the method, such as

w->Wait();
delete w;

What I'd do is have the Completed() method simply enqueue a request to the completion
handler thread when the processing was completed., and it would pass a pointer to the
Waiter object in so it could then be deleted. Or it might simply call
        delete this;
internally.

Note that the need to do synchronization on the array is unnecessary. Actually, it should
be unnecessary anyway, since the element of the array is all that is being manipulated,
and it is manipulated outside any concurrent access. (If it isn't, you are already in such
deep trouble that there is no recovery, because it means that two or more threads are
going to try to access the array element at the same time).

*****
    
>to process all downloaded pages together.
>
>Then in thread B startup
>pEvent->createBlockingEventSemaphore(SEM_BLOCKING,C_Total,1);
>
>wheer C_Total is the total number of web pages I must retrieve.
>
>B then loops creating C's upto the maximum concurrent C allowed.
>
>Then as each C terminates (end of threadrun())
>
>pEvent->maintainBlockingSemaphores(-1);
>
>Note I haven't shown the concurrent C limit semaphore code as I use that in
>a conventional manner.
>
>
>Does the above look OK? I haven't seen this type of code structure used
>anywhere so I am not 100% sure it is safe.
>
>I hope the above makes sense, but its v late where I am and I'm knackered ;)
>If you need any more info just let me know.
>
>Cheers
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>

Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

Relevant Pages

  • Re: output.c error in multithreaded program
    ... There would be no need for a global semaphore under any ... Just let A build the requests and go away, ... you would just queue ... Why have an array of handles and an array of counts when the ...
    (microsoft.public.vc.mfc)
  • Re: Task goes into a Pend+I state : Semaphore issue?
    ... problem.The common resource to be protected is the queue of commands. ... put the cmd in the queue. ... The issue in previous case 'could be' because of same semaphore ID ... used by some other task of priority 20 or temp. ...
    (comp.os.vxworks)
  • Re: WaitForMultipleObjects + Semaphore Makes Me Uncomfortable
    ... bothering me regarding WaitForMultipleObjects when semaphores are ... synchronization objects is a semaphore guarding a queue. ... a feature (the order of objects in the array of handles can have meaning). ...
    (microsoft.public.win32.programmer.kernel)
  • Re: Lahman, how ya doing?
    ... >> A priority queue is an interesting idea. ... So Timer just enqueues the event on the right queue. ... >effectively starts at the same time on the current tick. ... >was triggered just gets time-sliced based on priority. ...
    (comp.object)
  • Re: Lahman, how ya doing?
    ... Suppose you had 64 Thermometers and at run time you wanted to give priority to some of them dynamically (e.g., where the temperature gradient was greatest) because processing all 64 samples at once can't be done in a single "tick". ... So Timer just enqueues the event on the right queue. ... So I addressed both the bitmap processing basics via indexed OR mask and the deferredBitmap variable assumption about bit count. ...
    (comp.object)