On May 30, 2011, at 8:15 PM, Bing Li wrote:
> - (NSString *) receiveMessage
> {
> NSMutableString *receivedString;
> NSString *message;
> NSString *newReceivedString;
> [isConnectedLock lock];
> @try
> {
> if (isConnected)
> {
> char buffer[Constants.WWW.BUFFER_SIZE];
>
> // Receive remote data
> ssize_t numBytesReceived = recv(destinationSocket,
> buffer, Constants.WWW.BUFFER_SIZE, 0);
> if (numBytesReceived <= 0)
> {
> return Constants.WWW.NO_RECEIVED_MESSAGE;
> }
> buffer[numBytesReceived] = '\0';
>
>
> // Convert data to NSString locally
> receivedString = [[NSMutableString alloc]
> initWithBytes:buffer length:numBytesReceived encoding:NSUTF8StringEncoding];
> message = [[NSString alloc]
> initWithString:Constants.WWW.EMPTY_STRING];
Why are you storing a non-autoreleased object to a pointer and then writing
over it later?
>
>
> // Since the data has specific formats, some simple
> parsing jobs by delimiter are done as follows.
>
> NSRange range = [receivedString
> rangeOfString:Constants.WWW.DELIMITER];
> if (range.location > 0 && range.location <
> [receivedString length])
> {
> message = [receivedString
> substringToIndex:range.location];
Here you're overwriting a pointer to an object you own with an autoreleased
object.
>
> // Return data after parsing
> return message;
> }
> else
> {
> // Receive remote data continually if the
> receivedString is longer than expected
> while (numBytesReceived > 0)
> {
> NSAutoreleasePool *loopPool =
> [[NSAutoreleasePool alloc] init];
>
> // Receive data
> numBytesReceived =
> recv(destinationSocket, buffer, Constants.WWW.BUFFER_SIZE, 0);
> if (numBytesReceived <= 0)
> {
> return
> Constants.WWW.NO_RECEIVED_MESSAGE;
Here you're leaking the autorelease pool you created above.
> }
>
>
> buffer[numBytesReceived] = '\0';
> newReceivedString = [[[NSString
> alloc] initWithBytes:buffer length:numBytesReceived
> encoding:NSUTF8StringEncoding] autorelease];
> range = [receivedString
> rangeOfString:Constants.WWW.DELIMITER];
> if (range.location > 0 &&
> range.location < [receivedString length])
> {
> message = [receivedString
> substringToIndex:range.location];
>
> // Return data after parsing
> return message;
Here you're placing a new object in the autorelease pool, and then leaking the
autorelease pool.
> }
> [loopPool drain];
> }
> }
> return Constants.WWW.NO_RECEIVED_MESSAGE;
> }
> else
> {
> return Constants.WWW.NO_RECEIVED_MESSAGE;
> }
> }
> @catch (NSException *e)
> {
> return Constants.WWW.NO_RECEIVED_MESSAGE;
> }
> @finally
> {
> [isConnectedLock unlock];
>
> // Before returning received data, autorelease them here.
> [receivedString autorelease];
> [message autorelease];
Here you're autoreleasing a pointer that may or may not have been autoreleased
already.
I suggest you re-read the memory management rules again and pay attention to
the rules regarding release and autorelease. If anyone tries to re-state it
here, we're inevitably going to get something wrong.
<http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/MemoryMgmt/MemoryMgmt.html>
Also, I cannot recommend writing an object to a pointer that the program is
never going to read. If you're concerned about accidentally accessing an
uninitialized pointer, then just set it to nil instead.
Nick Zitzmann
<http://www.chronosnet.com/>
_______________________________________________
Cocoa-dev mailing list (Cocoa-dev@lists.apple.com)
Please do not post admin requests or moderator comments to the list.
Contact the moderators at cocoa-dev-admins(at)lists.apple.com
Help/Unsubscribe/Update your Subscription:
http://lists.apple.com/mailman/options/cocoa-dev/archive%40mail-archive.com
This email sent to arch...@mail-archive.com
