Possible Leak

[javi830810]

https://github.com/TykTechnologies/tyk/blob/c607854b9d50d40b79695e9f60a50c47a356e3f8/coprocess_python.go#L247-L254

Should the IF in line 247 evaluate true because of any reason. The release memory a few lines below wont execute, that will mean a Leak.

Also this happens many different times in the Coprocess and C integration. The correct way to use this release methods is by using a defer C.free(....

[matiasinsaurralde]

Hi @javi830810, thanks for this report. We'll issue a fix for this, the impact isn't severe as a dispatcher initialization error means that isn't not possible to use the Python build at all and the user will need to fix the issue and start the process again.

[javi830810]

Hi, we're investigating a leak in Python middlewares, we're reading code, running Performance tests, etc... One of the few things we've noticed is the absence of usage for PY_DECREF, when creating objects in CPython, like here:

https://github.com/TykTechnologies/tyk/blob/master/coprocess_python.go#L127-L155

Any reason for that? We've seen lots of improvements in memory by decreasing references there. Also, do you guys have any confirmation for a Memory Leak regarding Python Middlewares? We can reproduce the leak easily.

[matiasinsaurralde]

We don't have any report, can you share more details on memory improvements / issues? Best

[javi830810]

Below is attached a config (tyk.conf+middleware) where you can easily reproduce the leak, just hit it with ~ 50 user for about 4 minutes (around 70k requests) you'll see how it goes from around 20 mbs to about 200mbs.

Clarify: This tyk.conf is production ready, with all the bells and whistles, disabled_health checks etc... We might be switching gears to GRPC because of this leak.

leak-config.zip

PS: Thanks for attending to this matias, really this is a lot PS2: This is tyk 2.3.8, with tyk 2.3.10 is worse

[buger]

@javi830810 is it possible for you to verify that #1231 fixes your issue?

[javi830810]

@buger The leak is confirmed even after those changes. In fact those changes (dismissing the fact that @matiasinsaurralde added one extra C.free), were already there, just missing the defer. I was able to narrow the leak to the very method the Python hook is dispatched.

I carefully analyzed line by line what's happenning in there, and the result is the code below. Which includes fixes like:

1. Changing PyTuple_Pack for PyBuild because of some people mentioning reference count increasing weirdly with PyTuple_Pack.
2. Adding PyDecref for those objects that were created in here and not needed elsewhere.

Note1: The code below still leaks...but less (like 5mb less per 5000 requests according to our testing) Note2: The code belongs to coprocess_python.go Note3: We totally abandoned Python Middlewares at this point, we changed our whole Middleware base into GRPC.

I hope this helps the community

static struct CoProcessMessage* Python_DispatchHook(struct CoProcessMessage* object) {
    struct CoProcessMessage* outputObject = malloc(sizeof *outputObject);

    if (object->p_data == NULL) {
        return outputObject;
    }

    gilState = PyGILState_Ensure();

    PyObject *oldRequestPy = PyBytes_FromStringAndSize(object->p_data, object->length);
    PyObject *hookResult = PyObject_CallFunction( dispatcher_hook, "(O)" , oldRequestPy );

    if( hookResult == NULL ) {
        PyErr_Print();
                Py_XDECREF(oldRequestPy);
        return outputObject;
    }

    PyObject *newRequestPy = PyTuple_GetItem(hookResult, 0);
    char *requestString = PyBytes_AsString(newRequestPy);
    PyObject *requestLengthPy = PyTuple_GetItem(hookResult, 1);
    long requestLength = PyLong_AsLong(requestLengthPy);

    outputObject->p_data = requestString;
    outputObject->length = requestLength;

    Py_XDECREF(oldRequestPy);

    PyGILState_Release(gilState);
    return outputObject;
}

[matiasinsaurralde]

Thanks for the suggestions @javi830810, we'll analyze them in the following days.

[buger]

@matiasinsaurralde prepared uber fix for lot of bundler issues, which should handle this leak too. @javi830810 it will be really helpful if you can confirm that https://github.com/TykTechnologies/tyk/pull/1260 fix your issue. Thanks!

[lonelycode]

@buger Is this closed? Issue seems very old.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs, please add comments to this ticket if you would like it to stay open. Thank you for your contributions.