From: Dale H. <da...@wa...> - 2001-07-15 15:47:29
|
Chuck, I dunno if two threads can change data at the same time in cpQueue. I think that's why the mutex is in there. But, I don't think there should be a wait mutex to cause the caller to pause if the queue is full. This will block the caller. He may have other resources tied up at the time. Who knows what sort of deadlocks might occur. The basic plan was to throw away data that could not be put in the queue, not wait until space was available. What happens to the data depends on the state of the "dyn" variable. If TRUE then the memory containing the data is freed. I should have also set the pointer to NULL but didn't. If "dyn" is false the data is simply ignored. The dyn setting is set at the time the queue is created. Potential pitfall... If the caller puts an item on the queue then uses the data afterwards he will be in trouble if the queue was full and the memory was freed. Once data is put on the queue it should be considered gone forever by the caller if the queue has the "dyn" flag set. Only the queue reader should access it. The queue reader must free the memory or pass it to another function that does. Only one queue reader is allowed. Stuff that needs work: delete needs [] Pointer should be set to NULL after delete. Speaking of "delete[]"..... Actually I'm still a bit confused about delete[]. A char* is an array and needs the []. A string object is what? Internally it's an array but it was not declared an array so I assume it gets a plain "delete" ? aprsString would also get a plain "delete"? On Saturday 14 July 2001 15:49, Chuck Byam wrote: > On Friday 13 July 2001 19:54, you wrote: > :: The killer bug that will not die. (sigh) > :: I had high hopes that fixing the > :: unterminated string problem was gonna really help. > :: > :: On Friday 13 July 2001 17:33, Chuck Byam wrote: > :: > On Friday 13 July 2001 15:55, you wrote: > :: > :: I see it's been running 1.1 hours now. good start. > :: > > :: > Well... after about 2.5 hours first is chewing up the CPU cycles. > :: > I'ts still accepting connections and handing out data, but one of the > :: > threads is hogging the CPU (88% with top running). > > Your changes may very well have fixed an issue, that being the segfaults. > What this is, I think is a race condition that occurs between two (or more) > threads. I've been looking at the cpqueue code and trying to figure out if > it's possible for two threads to change data there at the same time. Look > at the attached and let me know if it make any sense to you. It > essentially provides wait variables that has the caller wait until a > condition is true, in this case whether the queue is full or empty. > > Chuck > > int cpQueue::write(char *cp, int n) > { > int rc=0; > > if (lock) > return -2; // Lock is only set true in the > destructor > > if(pthread_mutex_lock(mut) != 0) > cerr << "Unable to lock mut - cpQueue:Write-char *cp.\n" << flush; > > inWrite = 1; > int idx = write_p; > > while (base_p[idx].full) { > cerr << "Queue is full... waiting" << endl; > pthread_cond_wait(base_p[idx].notFull, mut); > } > > if (base_p[idx].rdy == false) { // Be sure not to overwrite old > stuff > base_p[idx].qcp = (void*)cp; // put char* on queue > base_p[idx].qcmd = n; // put int (cmd) on queue > base_p[idx].rdy = true; // Set the ready flag > base_p[idx].empty = false; > idx++; > itemsQueued++; > if (itemsQueued > HWitemsQueued) > HWitemsQueued = itemsQueued; > > if (idx >= size) > idx = 0; > > write_p = idx; > } else { > overrun++ ; > > if (dyn) > delete cp; > > rc = -1; > } > > inWrite = 0; > > if(pthread_mutex_unlock(mut) != 0) > cerr << "Unable to unlock mut - cpQueue:Write - char *cp.\n" << > flush; > > pthread_cond_signal(base_p[idx].notEmpty); > return(rc); > } > > void* cpQueue::read(int *ip) > { > if(pthread_mutex_lock(mut) != 0) > cerr << "Unable to lock mut - cpQueue:read - int.\n" << flush; > > while (base_p[read_p].empty) { // wait here if the queue is empty > cerr << "Queue empty... waiting." << endl; > pthread_cond_wait(base_p[read_p].notEmpty, mut); > } > > inRead = 1; > void* cp = base_p[read_p].qcp ; // Read the TAprsString* > > if (ip) > *ip = base_p[read_p].qcmd ; // read the optional integer > command > > base_p[read_p].qcp = NULL; // Set the data pointer to NULL > base_p[read_p].rdy = false; // Clear ready flag > read_p++; > itemsQueued--; > > if (read_p >= size) > read_p = 0; > > inRead = 0; > > if (pthread_mutex_unlock(mut) != 0) > cerr << "Unable to unlock mut - cpQueue:read - int.\n" << flush; > > pthread_cond_signal(base_p[read_p].notFull); > > return(cp); > } -- Dale Heatherington da...@wa... Web Page http://www.wa4dsy.net Sent by KMail for Linux |
From: Chuck B. <cb...@vi...> - 2001-07-15 17:59:59
|
On Sunday 15 July 2001 11:47, Dale Heatherington wrote: :: Chuck, :: I dunno if two threads can change data at the same time in cpQueue. I :: think that's why the mutex is in there. But, I don't think there :: should be a wait mutex to cause the caller to pause if the queue is :: full. This will block the caller. He may have other resources tied up :: at the time. Who knows what sort of deadlocks might occur. :: << snip, snip >> Just reaching here. Deadlocks are the problem though. When I run first from the console eventually I'll see messages of not being able to create a new client thread. This occurs in TCPSessionThread and results from rc being != 0 from a call to pthread_create (either tcp server or http thread). << snip, snip >> :: Speaking of "delete[]"..... :: Actually I'm still a bit confused about delete[]. :: A char* is an array and needs the []. :: A string object is what? Internally it's an array :: but it was not declared an array so I assume :: it gets a plain "delete" ? aprsString would also get :: a plain "delete"? :: In C++ there are two kinds of pointers, pointers to a single object and pointers to an array of objects. The important thing is if your call to new uses [], so should your call to delete. Chuck |