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

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

Open
danielcjacobs opened this issue Mar 7, 2023 · 0 comments

Comments

@danielcjacobs
Copy link
Contributor

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.

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

No branches or pull requests

1 participant