Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Wolf Vollprecht
    @wolfv

    thanks for the replies! I am going to add you just so you get notifications. There won't be any work until you make a release, and even then hopefully everything will work smoothly :)

    The rt fix is something I had to do a couple of times already for conda-recipes. I am not sure exactly why it's needed when compiling with the conda-compilers, vs. the regular OS compilers. Maybe the add -lrt implicitly?

    I just added it to the tests, which means that people who want to use the timeout functionality will have to link themselves with rt.
    Do you think linking the .so would be better, instead of the testts?
    Wolf Vollprecht
    @wolfv
    this is the feedstock now: https://github.com/conda-forge/reproc-feedstock from where new builds will be pushed to the conda-forge repository
    Daan De Meyer
    @DaanDeMeyer
    I'm curious what the exact failure is when rt is missing, I'm assuming I used some symbol from rt but I can't immediately figure out which one that would be
    Wolf Vollprecht
    @wolfv
    clock_gettime :)
    Daan De Meyer
    @DaanDeMeyer
    Ahhh, alright, I'll just add that to reproc.
    Daan De Meyer
    @DaanDeMeyer
    I pushed a commit that links against librt on unix (but not osx) platforms. Can you give it a test?
    Brad Larsen
    @bradlarsen

    Hey @DaanDeMeyer -- useful project! I have been using reproc in an application, and have noticed a couple issues:

    (1) The ASSERT_UNUSED macro is used in several places for error handling instead of checking function contracts or invariants. The end result is that in debug builds, sometimes reproc crashes the application with an assertion failure for something that is actually just an uncommon but valid return value.

    (2) The EINTR error code, used for interrupted system calls, isn't handled explicitly by reproc at all. Using reproc++, I have seen things like process creation failing, and drain failing, due to interrupted system calls. In these places I have been able to put a do { ec = ... } while (ec.value() == EINTR); around the call in my application, but I was wondering if this is something you would want to handle in the reproc / reproc++ libraries themselves.

    I can make a couple PRs if you'd like, but I figured I would check with you first!
    Daan De Meyer
    @DaanDeMeyer
    (1) is 100% bugs on reproc's side. The asserts should always be true, if they aren't, I've made an incorrect assumption somwhere and that should be fixed
    I'd definitely appreciate a PR for those
    (2) is more complex, sometimes users might want to handle EINTR themselves instead of relying on reproc to do it. Usually, you'd get around this by specifying SA_RESTART in a call to sigaction when you want to restart the system call on EINTR for a specific signal. Would that work for your use case? If not, there's no harm in adding an option to reproc that tells it to retry system calls with the usual do while approach.
    Daan De Meyer
    @DaanDeMeyer
    It would introduce a bit of churn since every function doing a system call would need to take a parameter telling it to retry system calls but that might be unavoidable.
    Daan De Meyer
    @DaanDeMeyer
    Of course, this setting is global per signal but that might work for your use case
    Daan De Meyer
    @DaanDeMeyer
    I looked again and now I remember fully, I use ASSERT_UNUSED in scenarios where I can't report the error as a return value to the user. This is usually in cleanup functions or after forking when spawning a process. My idea was that these were only for DEBUG builds anyway and making these asserts might help me figure out if these commands could fail or not. I would not remove the asserts but rather modify them to ignore the errors you're seeing and add a comment to them explaining why these errors could happen.
    Brad Larsen
    @bradlarsen
    @DaanDeMeyer I am back from holiday vacation now and will put together a PR for the assertion failures in the next couple weeks.
    Most of the places where I have seen ASSERT_UNUSED fail was because system calls could fail with EINTR
    Daan De Meyer
    @DaanDeMeyer
    Thinking more about this, I think we can retry EINTRs for system calls that we don't report errors for (those that are followed by ASSERT_UNUSED).
    None of those are expected to block very long anyway
    Daan De Meyer
    @DaanDeMeyer
    Actually, don't retry close() when EINTR occurs, see https://bugs.chromium.org/p/chromium/issues/detail?id=269623
    So just ignore EINTR there. The rest should be safe to retry
    Brad Larsen
    @bradlarsen
    Great; thanks for the pointers. I'm gonna try to get to this later this week.
    AokiYuune
    @AokiYuune
    Good Night! I'm a beginner of reproc. Just
    for learning.