-1

This long code is sending a POST request using a Socket, the whole code works without any complain, but what i'm facing right now is, that it eating a lot of cpu power, uses too much memory (RAM)

I can see that, because my laptop is getting hot very fast and by looking checking at my mac.

i have tried to find the bug or where , that very large memory Issue is, but couldn't. I have spent over a month trying to fix it by my own, but don't know what i'm doing to be honest.

i was so desperate, that i putted all kind of freeing metod.. even that it wrong.. just to see if that made a change, but nothing did..

So now i don't know what wrong or how to fix this, please help me out…

Going update the code with select, moment... trying to clean it up first

user1417815
  • 445
  • 3
  • 7
  • 16
  • 1
    Here's a trick: use only one of C or C++. This code is a bad bad bad bad mixture of the two. It doesn't invoke undefined behaviour only *because it has a leak* and the bad cleanup code never runs. – R. Martinho Fernandes May 30 '12 at 10:38
  • I agree with you, but i'm trying to fix the big memory leak first – user1417815 May 30 '12 at 10:40
  • The problem is in Database() function - the memleak at the end, see my answer. Also you might clean the code by removing the strip_copy() which is not used in these routines. – Viktor Latypov May 30 '12 at 10:41
  • I'm still confused on how to fix it.. – user1417815 May 30 '12 at 10:44
  • 3
    I put some code in the answer below. Dont be offended, but this code is quite bad, you should look into reading some beginner c++ books :) Here is an excellent one: http://www.mindview.net/Books/TICPP/ThinkingInCPP2e.html – Brady May 30 '12 at 10:48
  • The best fix: [stop using `new`](http://stackoverflow.com/questions/8839943/why-does-the-use-of-new-cause-memory-leaks-in-c/8840302#8840302) (and malloc). No leaks, no undefined behaviour. – R. Martinho Fernandes May 30 '12 at 10:51
  • Brady its okay, you doing what you can :) - R. Martinho Fernandes true – user1417815 May 30 '12 at 11:05

2 Answers2

4

First thing: do NOT mix malloc() and delete[]. They might or might not refer to the same memory allocator. So use the malloc()/free() or new char[]/delete[] pairs.

The problems is here (in Database() function): you've got a terrible memleak. Do not allocate memory like this for result passing. Better use buffers. You program is multithreaded so use the buffers on the stack. Do NOT call delete[] on anything that is not allocated by you (var declaration like "char Buf[100];" is not an allocation).

New version (I omitted main() and strip() functions. Also the includes):

#define MAX_ECHO_SIZE (1024)
#define MAX_RESPONSE_SIZE (1024)

void process_http(int sockfd, const char *host, const char *page, const char *poststr, char* OutResponse)
 {
    char* ptr;
    char sendline[MAXLINE + 1], recvline[MAXLINE + 1];
    ssize_t n;

    snprintf(sendline, MAXSUB, 
         "POST %s HTTP/1.0\r\n"  // POST or GET, both tested and works. Both HTTP 1.0 HTTP 1.1 works, but sometimes 
         "Host: %s\r\n"     //oth HTTP 1.0 HTTP 1.1 works, but sometimes HTTP 1.0 works better in localhost type
         "Content-type: application/x-www-form-urlencoded\r\n"
         "Content-length: %d\r\n\r\n"
         "%s\r\n", page, host, (unsigned int)strlen(poststr), poststr);

    if (write(sockfd, sendline, strlen(sendline))>= 0) 
    {
        while ((n = read(sockfd, recvline, MAXLINE)) > 0) 
        {
            recvline[n] = '\0';

            if(fputs(recvline,stdout) ==EOF) { cout << ("fputs erros"); }
            ptr = strstr(recvline, "\r\n\r\n");
            strip(ptr, "\r\n\r\n");

            // check len for OutResponse here ?
            snprintf(OutResponse, 6000,"%s", ptr);
        }
    }
}

int Database( const char * hname,  const char * page,  const char * var, const char * poststr,  int  port, char* EchoResponse, int MaxEchoLen){

    char url[MAXLINE];
    char response[MAX_RESPONSE_SIZE];

    snprintf(url, MAXLINE, "%s=%s", var, poststr);

    short int sockfd ;
    struct sockaddr_in servaddr;
    struct hostent *hptr;
    char str[MAXLINE];
    char** pptr;

    hptr = gethostbyname(hname);

    sockfd = socket(AF_INET, SOCK_STREAM, 0);

    if (!hptr) {
        cout << ("host not found\n");
        return -1; // "host not found";
    }

    if (hptr->h_addrtype == AF_INET && (pptr = hptr->h_addr_list) != NULL) {
        inet_ntop(hptr->h_addrtype, *pptr, str, sizeof(str)); 
    }
    if (sockfd  >= 0  ) {
        bzero(&servaddr, sizeof(servaddr));
        servaddr.sin_family = AF_INET;
        servaddr.sin_port = htons(port);
        inet_pton(AF_INET, str, &servaddr.sin_addr);

        if (connect(sockfd, (SA *) & servaddr, sizeof(servaddr)) < 0) {
            return -2; // "Server down, connect error";
        }
        else {
            process_http(sockfd, hname, page, url, &response[0], MAX_RESPONSE_SIZE);

            int len = strlen(response)+1;
            if(len >= MaxEchoLen) { return -3; /* buffer too small*/ }

            // Copy the contents with
            strcpy(EchoResponse, response);

            /// You must not free all of char[] allocated on stack
            close(sockfd);

            return 0; // OK
        }
    }
}

void *multithreading1( void *ptr ) {
    char LocalEchoResponse[MAX_ECHO_SIZE];

    while (1) {
            int RetCode = Database("2.107.xx.xxx", "/ajax.php", "submit", "HEllo WORLD", 80, &LocalEchoResponse[0], MAX_ECHO_SIZE);
            /// check the error
    }   
}
Viktor Latypov
  • 14,289
  • 3
  • 40
  • 55
  • Viktor How can that be changed to work.. without problem? – user1417815 May 30 '12 at 10:41
  • -1 Your quick "fix" invokes undefined behaviour! – R. Martinho Fernandes May 30 '12 at 10:46
  • I got: malloc: *** error for object 0x100100c7d: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug Abort trap – user1417815 May 30 '12 at 10:49
  • I don't see the Database() call yet to fix the memory. I can suggest using a global static buffer to return result, if multithreading is not an issue. Also I've just indicated the problem. There's no "quick" fix possible, strictly speaking. – Viktor Latypov May 30 '12 at 10:51
  • Double-check your allocations, we can't do all the work for you. Even the quick "fix" and other suggestions my lead to more complications :) – Viktor Latypov May 30 '12 at 10:54
  • I updated the whole code and posted how i'm using it, then you can see how it all working.. :) – user1417815 May 30 '12 at 11:02
  • R. Martinho Fernandes: Cleaned up the stuff, my first version was a mess. Didn't suspect that delete[]s where called on stack-allocated and libc-returned pointers. – Viktor Latypov May 30 '12 at 11:20
  • @ Viktor Thanks!!! but .. i dont get it to compile now, i tried your code. In case you want to see how i'm using it' i updated the code.. to you to see – user1417815 May 30 '12 at 12:17
  • Well, there might be some typos in, I was in a real hurry. The general idea - use buffers as parameters to get the results. Do not return that much pointers and do not try do delete[] everything. – Viktor Latypov May 30 '12 at 12:21
  • Changed: " process_http(sockfd, hname, page, url, &response[0], MAX_RESPONSE_SIZE); " to: " process_http(sockfd, hname, page, url, &response[0]); " and now it compile. Well its use much less memory, but still big very amount.... it was % cpu: 100-140 , now 60-98 – user1417815 May 30 '12 at 12:27
  • in your "int main()" you have a "while(true) {}" loop. Change it to "while(true) { usleep(1000); }". It should reduce the CPU usage dramatically. Otherwise it is just burning all the CPU cycles available. – Viktor Latypov May 30 '12 at 12:30
  • You are my HERO!!!!! :) thanks, thanks.. i wish a could do you a favor – user1417815 May 30 '12 at 12:32
  • In case of troubles, drop me a mail. And please, check your code carefully: most of the problems can be avoided with little effort. – Viktor Latypov May 30 '12 at 12:34
  • +1 for rewriting the process_http() It would have been better to include a call to select() too, but the OP can read about that too. – Brady May 30 '12 at 12:45
  • select() will remove another 90% of CPU consumption :) – Viktor Latypov May 30 '12 at 12:45
  • I'm going read about select(), you both guys did alot of work, now its time for some learning.. :) but i would wish that you included select() so i could understand it better how things works – user1417815 May 30 '12 at 12:49
  • Check this out for a WebServer sample: http://www.adp-gmbh.ch/win/misc/webserver.html Porting this to pure POSIX requires little effort. It does not use select(), however. But it is a very clean and simple code. – Viktor Latypov May 30 '12 at 12:50
  • Thanks for the link, going read about it right now, thans alot for the help i owe you some. :) Let hope i understand select() ... – user1417815 May 30 '12 at 12:54
  • @Viktor Latypov i'm confused with one thing right now, before i could send data and show date, but how do i read data now??? on screen, that what im getting back – user1417815 May 30 '12 at 21:37
  • I can see : // check len for OutResponse here ? snprintf(OutResponse, 6000,"%s", ptr);, but how do i get data? out of the function??? – user1417815 May 30 '12 at 21:54
  • You pass the pointer to the buffer. So the OutResponse is just a reference to an external memory block. In Database() you just have your response filled. Don't know how to explain, it's like "Oh, dear function, put your result here and do not return it as a return-value" – Viktor Latypov May 30 '12 at 21:58
  • Can you show how to pull the data and have it inside multithreading1?? – user1417815 May 30 '12 at 22:07
  • Can you please implant a way to have a special time-out amount? Like if nothing come from the server under 60 second, then say " Server down" i'm started a bounty, because you deserve it, brother! – user1417815 Jun 03 '12 at 12:01
  • @Viktor Latypov i'm still waiting you, please the bounty is going end soon.. – user1417815 Jun 06 '12 at 23:03
  • I'm going reward you, even that.. you are ignoring me.. :( – user1417815 Jun 06 '12 at 23:17
1

You have a pretty big memory leak in strip_copy. Whatever you put after the return will never get executed. Im surprised the compiler didnt complain about this. Same problem in the process_http() function.

Fix it like this:

static void strip_copy(char const *s, char *buf, const char * SPACE)
{
    if (buf)
    {
        char *p = buf;
        char const *q;
        int n;
        for (q = s; *q; q += n + strspn(q+n, SPACE))
        {
            n = strcspn(q, SPACE);
            strncpy(p, q, n);
            p += n;
        }
        *p++ = '\0';
        buf = (char*)realloc(buf, p - buf);
    }
}

// Then call it like this
char *buf = new[1 + strlen(s)];
strip_copy(s, buf, ' ');
// use buf
delete [] buf;

And in process_http()

const char*  process_http(int sockfd, const char *host, const char *page, const char *poststr)
{
    ....
    // delete here only what you dynamically 
    // allocated with new() BEFORE the return
    return response; // n
}

And DONT mix malloc() with delete():

  • malloc() goes with free()
  • new() goes with delete()

This isnt related to the memory used, but instead of calling read()/write() directly, you should use select() to know when its ready to be read or written. Here's a related question: https://stackoverflow.com/a/10800029/1158895

Community
  • 1
  • 1
Brady
  • 10,207
  • 2
  • 20
  • 59