lcm-proj / lcm

Lightweight Communications and Marshalling
GNU Lesser General Public License v2.1
944 stars 385 forks source link

Python implementation of `handle_timeout` doesn't match C++ implementation #401

Open danielcjacobs opened 1 year ago

danielcjacobs commented 1 year ago

This is the python wrapper for lcm_handle_timeout:

static PyObject *pylcm_handle_timeout(PyLCMObject *lcm_obj, PyObject *arg)
{
    int timeout_millis = PyInt_AsLong(arg);
    if (timeout_millis == -1 && PyErr_Occurred())
        return NULL;
    if (timeout_millis < 0) {
        PyErr_SetString(PyExc_ValueError, "invalid timeout");
        return NULL;
    }

    dbg(DBG_PYTHON, "pylcm_handle_timeout(%p, %d)\n", lcm_obj, timeout_millis);

    if (lcm_obj->saved_thread_state) {
        PyErr_SetString(PyExc_RuntimeError,
                        "Simultaneous calls to handle() / handle_timeout() detected");
        return NULL;
    }
    lcm_obj->saved_thread_state = PyEval_SaveThread();
    lcm_obj->exception_raised = 0;

    dbg(DBG_PYTHON, "calling lcm_handle_timeout(%p, %d)\n", lcm_obj->lcm, timeout_millis);
    int status = lcm_handle_timeout(lcm_obj->lcm, timeout_millis);

    // Restore the thread state before returning back to Python.  The thread
    // state may have already been restored by the callback function
    // pylcm_msg_handler()
    if (lcm_obj->saved_thread_state) {
        PyEval_RestoreThread(lcm_obj->saved_thread_state);
        lcm_obj->saved_thread_state = NULL;
    }

    if (lcm_obj->exception_raised) {
        return NULL;
    }
    if (status < 0) {
        PyErr_SetString(PyExc_IOError, "lcm_handle_timeout() returned -1");
        return NULL;
    }
    return PyInt_FromLong(status);
}

And this is the C++ wrapper:

inline int LCM::handleTimeout(int timeout_millis)
{
    if (!this->lcm) {
        fprintf(stderr, "LCM instance not initialized.  Ignoring call to handle()\n");
        return -1;
    }
    return lcm_handle_timeout(this->lcm, timeout_millis);
}

Note, the latter simply returns whatever lcm_handle_timeout, but the python implementation throws an IOError if status < 0.

I think the C++ implementation makes more sense, as it should be up to the user to decide what to do with the retval of lcm_handle_timeout. For example, when playing a log file, this function returns -1 at the end of the file. This isn't really worth throwing an exception when we can just return -1.

Should change the python wrapper to match the C++ wrapper. Likely would impact #201.