Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Memory Leak #4504

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

PradyumnAgrawal05
Copy link

Fix for #4261.

@fgalan
Copy link
Member

fgalan commented Feb 6, 2024

Looks promising! Thanks! :)

Note the fail in 0000_statistics_operation/statistics_with_counters.test detected by GitActions. You probably need to do some adjust in the --REGEXPECT-- section in that test (as the comment sais: "The REGEXPECT part would need a little adapation after that").

@PradyumnAgrawal05
Copy link
Author

Looks promising! Thanks! :)

Note the fail in 0000_statistics_operation/statistics_with_counters.test detected by GitActions. You probably need to do some adjust in the --REGEXPECT-- section in that test (as the comment sais: "The REGEXPECT part would need a little adapation after that").

Updated, please review.

@fgalan
Copy link
Member

fgalan commented Feb 8, 2024

Looks better! :)

Probably an extra adjustment is needed. I see this in test execution:

-----  statistics with counters  -----
(0000_statistics_operation/statistics_with_counters.test) output not as expected
VALIDATION ERROR: input line:
           "jsonRequests": 17,
does not match ref line:
           "jsonRequests": 16,

@fgalan
Copy link
Member

fgalan commented Feb 9, 2024

I'm afraid the validation error remains after the last modifications...

-----  statistics with counters  -----
(0000_statistics_operation/statistics_with_counters.test) output not as expected
VALIDATION ERROR: input line:
               "/v2/op/notify": {
does not match ref line:
               "/v2/op/query": {

Maybe this can be fixed reordering the keys in the JSON in the proper way.

Added OPTIONS in /v2/op/notify
@fgalan
Copy link
Member

fgalan commented Feb 9, 2024

I'm afraid the validation error remains after the last modifications...

-----  statistics with counters  -----
(0000_statistics_operation/statistics_with_counters.test) output not as expected
VALIDATION ERROR: input line:
           "invalidRequests": 1,
does not match ref line:
           "jsonRequests": 17,

I recommend you to run the tests locally until it gets ok.

@fgalan
Copy link
Member

fgalan commented Feb 15, 2024

I'm afraid the validation error remains after the last modifications...

-----  statistics with counters  -----
(0000_statistics_operation/statistics_with_counters.test) output not as expected
VALIDATION ERROR: input line:
           "invalidRequests": 1,
does not match ref line:
           "jsonRequests": 17,

I recommend you to run the tests locally until it gets ok.

If adjusting the test is getting hard to you, I can finalize the work. It would be great if we could complete this PR so we can produce a 3.11.1 hotfix for Orion.

Just tell me if you need help with this.

@PradyumnAgrawal05
Copy link
Author

Hi @fgalan,

Thank you for your guidance and offer to assist with the tests. I've updated the PR and ensured that all tests pass locally. Let's proceed to finalize it for the 3.11.1 hotfix for Orion.

@fgalan
Copy link
Member

fgalan commented Feb 16, 2024

I have done a test like this.

First, to check I'm using the last version in your branch (note git version commit matches the last one at the present moment in the PR):

(p3) fermin@bodoque:~/src/fiware-orion/test/valgrind$ contextBroker --version
3.11.0-next (git version: de08b9eefa1121e9d942947c9e7dbb93780238df)
Copyright 2013-2024 Telefonica Investigacion y Desarrollo, S.A.U
Orion Context Broker is free software: you can redistribute it and/or
modify it under the terms of the GNU Affero General Public License as
published by the Free Software Foundation, either version 3 of the
License, or (at your option) any later version.

Orion Context Broker is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero
General Public License for more details.

Telefonica I+D

Next, to run valgrindTestSuite.sh on the testing ftest:

(p3) fermin@bodoque:~/src/fiware-orion/test/valgrind$ ./valgrindTestSuite.sh statistics_with_counters.test
vie 16 feb 2024 12:46:30 CET
Run tests 0 to 0
./valgrindTestSuite.sh: línea 243: cd: test/valgrind: No existe el fichero o el directorio
Test 001/1: 0000_statistics_operation/statistics_with_counters ...................................................................................... FAILED (lost: 632). Check statistics_with_counters.valgrind.out for clues


1 tests leaked memory:
   001: 0000_statistics_operation/statistics_with_counters.test (lost 632 bytes, see statistics_with_counters.valgrind.out)
---------------------------------------

Total test time: 10.27 seconds (0 minutes)

A memory lost is detected (NOK).

Looking into the aforementioned statistics_with_counters.valgrind.out I see:

==169083== 632 (320 direct, 312 indirect) bytes in 1 blocks are definitely lost in loss record 46 of 53
==169083==    at 0x4840F2F: operator new(unsigned long) (vg_replace_malloc.c:422)
==169083==    by 0x323FA1: parseNotificationData(ConnectionInfo*, rapidjson::GenericMemberIterator<true, rapidjson::UTF8<char>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >, NotifyContextRequest*, OrionError*) (parseNotification.cpp:113)
==169083==    by 0x3251AF: parseNotificationNormalized(ConnectionInfo*, NotifyContextRequest*, OrionError*) (parseNotification.cpp:215)
==169083==    by 0x325CF4: parseNotification[abi:cxx11](ConnectionInfo*, NotifyContextRequest*) (parseNotification.cpp:252)
==169083==    by 0x30197C: jsonRequestTreat(ConnectionInfo*, ParseData*, RequestType, JsonDelayedRelease*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (jsonRequestTreat.cpp:168)
==169083==    by 0x2B5226: payloadParse(ConnectionInfo*, ParseData*, RestService*, JsonRequest**, JsonDelayedRelease*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (RestService.cpp:199)
==169083==    by 0x2B7476: restService(ConnectionInfo*, RestService*) (RestService.cpp:622)
==169083==    by 0x2B8797: orion::requestServe[abi:cxx11](ConnectionInfo*) (RestService.cpp:774)
==169083==    by 0x2B1A49: connectionTreat(void*, MHD_Connection*, char const*, char const*, char const*, char const*, unsigned long*, void**) (rest.cpp:1644)
==169083==    by 0x438F11: call_connection_handler (connection.c:3071)
==169083==    by 0x43A8C7: MHD_connection_handle_idle (connection.c:4673)
==169083==    by 0x43CE0E: call_handlers (daemon.c:1221)

Could you have a look, please?

You can to the same check with valgrindTestSuite.test to check yourself.

@PradyumnAgrawal05
Copy link
Author

Hi @fgalan,

I've implemented various approaches to address the memory leak issue, as outlined below (one previously shared also):
Out of those below are the two approaches mentioned:
Approach 1:

  for (rapidjson::Value::ConstValueIterator iter2 = iter->value.Begin(); iter2 != iter->value.End(); ++iter2)
  {
    ContextElementResponse* cerP = new ContextElementResponse();

    ncrP->contextElementResponseVector.vec.push_back(cerP);

    if (parseContextElementResponse(ciP, iter2, cerP, oeP) == false)
    {
        // Clean up allocated memory before returning in case of failure
        deleteAllocatedMemory(ncrP->contextElementResponseVector.vec);
        return false;
    }
  }

    // No memory leak if successful, do not delete here

  return true;
}

// Function to delete allocated memory for ContextElementResponse objects
static void deleteAllocatedMemory(std::vector<ContextElementResponse*>& vec)
{
    for (auto cerP : vec) {
        delete cerP;
    }
    vec.clear(); // Clear the vector
}

                                                                                           

Approach 2:

  for (rapidjson::Value::ConstValueIterator iter2 = iter->value.Begin(); iter2 != iter->value.End(); ++iter2)
  {
    ContextElementResponse* cerP = new ContextElementResponse();

    ncrP->contextElementResponseVector.vec.push_back(cerP);

    if (parseContextElementResponse(ciP, iter2, cerP, oeP) == false)
    { 
        delete cerP; // Deletes the allocated memory for each ContextElementResponse object if parsing fails
        for (ContextElementResponse* response : ncrP->contextElementResponseVector.vec) {
            delete response; // Deletes each ContextElementResponse object in the vector
        }
        ncrP->contextElementResponseVector.vec.clear(); // Clears the vector after deleting all elements
        return false;
    }
  }

  return true;
}

The memory leak issue still persists even after updating the REGEXPECT part. While the functional tests pass successfully, testing with Valgrind reveals that the issue remains unresolved.

Please guide me to proceed further.

@fgalan
Copy link
Member

fgalan commented Feb 29, 2024

Thanks for the analysis. I would have a look into it and provide feedback.

(Unfortunately, this wasn't ready for Orion 3.12.0 version, which we have released today, instead of 3.11.1. But it would be included for sure in a next Orion version :)

@fgalan
Copy link
Member

fgalan commented Sep 11, 2024

PR #4603 is doing a large refactor that probably affect this. Maybe after the rework the issue gets solved.

Let's wait until finishing PR #4603, then let's go back to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants