Out-of-memory failures results in confusing or non-sensible tracebacks.

[fake-name]

I've been tracking down an issue I've encountered on 512 MB VPSes (virtual private servers), and it's appears to be coming from msgpack:

 File "/scraper/client.py", line 336, in partial_capture
    self.put_message_chunked(response, connector)
  File "/scraper/client.py", line 443, in put_message_chunked
    message_bytes = msgpack.packb(message, use_bin_type=True)
  File "/scraper/venv/lib/python3.4/site-packages/msgpack/__init__.py", line 47, in packb
    return Packer(**kwargs).pack(o)
SystemError: error return without exception set

This can be replicated pretty easily. In a VM with 512 MB RAM and no swap:

durr@memtest:~$ python3
Python 3.5.2 (default, Nov 17 2016, 17:05:23)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> import msgpack
>>> data = os.urandom(1024 * 1024 * 175)
>>> message_bytes = msgpack.packb(data, use_bin_type=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.5/dist-packages/msgpack/__init__.py", line 47, in packb
    return Packer(**kwargs).pack(o)
SystemError: <built-in method pack of msgpack._packer.Packer object at 0x7f11af1e27b8> returned NULL without setting an error

I think the minor traceback differences are due to python differences - 3.4 for the original traceback, 3.5 for the example.

In any event, ideally the packer should raise something like a custom sub-class of MemoryError rather then an apparently broken SystemError, but at least documenting that packing a message can result in a failure might be nice too.

Interestingly enough, the fallback version seems to use much less memory, and produce the expected MemoryError on failure.

[fake-name]

Digging into the code, I think the problem is how Packer()._pack() is handling it's return code.

Adding a debug print to _pack(), it's returning -1, which in pack() should be treated as a MemoryError. However, no code after the _pack() call is getting executed, I think because _pack() is declared cdef int _pack(self, object o, int nest_limit=DEFAULT_RECURSE_LIMIT) except -1:. The except -1 is causing an exception to be raised, that's not handled by the calling context.

Why is that declared except -1? The calling code is explicitly handling the -1 return case, so there should be no need to have cython do anything.

Additionally, removing except -1 from the _pack() declaration leads to the code properly raising a MemoryError. I don't think this ever worked for the memory error case.

From the cython docs:

Furthermore, whenever a call to spam returns -1, an exception will be assumed to have occurred and will be propagated.

[fake-name]

Note: There is a similar implementation pattern in _unpacker.pyx. You have cdef int PyObject_AsReadBuffer(object o, const void** buff, Py_ssize_t* buf_len) except -1, and then when it's used, the return value is checked against -1.

        if PyObject_AsReadBuffer(obj, <const void**> buf, buffer_len) == -1:
            raise BufferError("could not get memoryview")

This will have the same issue, where what should be raised as a BufferError will instead wind up as a SystemError, since cython will implicitly raise an exception on a -1 return that then goes unhandled, based on the assumption that the only context in which the function can return -1 is a context where it has already set a python error before returning.

---

I'm not sure how to fix this properly, since the except -1 parameter does multiple things. The coercion of python exceptions to a return code is needed, but the automatic exception raising from the return code being checked is not.

Simply checking ret in _pack() before returning would work, at the cost of 2 duplicate lines of code.

[jfolz]

Seems like msgpack_pack_write only returns -1 on PyMem_Realloc failure, so a MemoryError must be raised instead.

Documentation on PyObject_AsReadBuffer states:

Returns -1 and sets a TypeError on error.

So in this case TypeError with a proper message should be raised.

The exception-situation is a bit wonky and should be fixed for the 1.0 release. See #233.

[jfolz]

Using more memory may have two causes:

• Native Packer objects allocate 1MB of memory when created.
• PyBytes_FromStringAndSize copies the used part of the internal buffer of the Packer before it is deallocated, so briefly twice as much memory may be used. The standard libraries may do something clever along the lines of _PyBytes_Resize to reuse the same block of memory.

[fake-name]

Working around the memoryerror was pretty straight forward, I just added more swap.

Mostly, the major reason I filed the issue was because there was nothing on google about this particular error from msgpack, and maybe this way it'll prevent anyone from having to go through all the detective work again.

[fake-name]

Seems like msgpack_pack_write only returns -1 on PyMem_Realloc failure, so a MemoryError must be raised instead.

Yeah, but from what I can see, PyMem_Realloc specifically doesn't set an exception on realloc failure, so what's happening is it's failing, and then the except -1 is blindly assuming an exception is set, and internally attempting to raise an unset exception, leading to the original error.

Basically, either _pack() and _unpack() need to handle the PyMem_Realloc() failure before returning, or the check of the PyMem_Realloc() needs to explicitly set an exception.

[fake-name]

Ok, I think the correct fix is to have msgpack_pack_write call PyErr_SetString:

static inline int msgpack_pack_write(msgpack_packer* pk, const char *data, size_t l)
{
    char* buf = pk->buf;
    size_t bs = pk->buf_size;
    size_t len = pk->length;

    if (len + l > bs) {
        bs = (len + l) * 2;
        buf = (char*)PyMem_Realloc(buf, bs);
        if (!buf)
        {
            PyErr_SetString(PyExc_MemoryError, "Out of memory exception!");
            return -1;
        }
    }
    memcpy(buf + len, data, l);
    len += l;

    pk->buf = buf;
    pk->buf_size = bs;
    pk->length = len;
    return 0;
}

This yields a sensible traceback:

Traceback (most recent call last):
  File "memtest.py", line 12, in <module>
    message_bytes = msgpack.packb(data, use_bin_type=True)
  File "/media/Storage/Scripts/msgpack-python/msgpack/__init__.py", line 53, in packb
    return Packer(**kwargs).pack(o)
  File "msgpack/_packer.pyx", line 266, in msgpack._packer.Packer.pack
    cpdef pack(self, object obj):
  File "msgpack/_packer.pyx", line 268, in msgpack._packer.Packer.pack
    ret = self._pack(obj, DEFAULT_RECURSE_LIMIT)
MemoryError: Out of memory exception!

Mostly, the point was, the -1 return value of _pack() is never propagated to the caller, since cython checks it and raises an exception if it's set, so the check in pack() is pointless:

        cdef int ret
        ret = self._pack(obj, DEFAULT_RECURSE_LIMIT)
        if ret == -1:
            raise MemoryError

[jfolz]

Yeah, but from what I can see, PyMem_Realloc specifically doesn't set an exception on realloc failure

I agree. I meant "must" as in "doesn't happen, but should" :)
As opposed to PyObject_AsReadBuffer "should" raise TypeError, because I assume it will but haven't actually tested that ;)

so the check in pack() is pointless

And all other pack* methods. Same should also hold for the check around PyObject_AsReadBuffer.