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

setitimer, getitimer wrapper #46493

Closed
gpolo mannequin opened this issue Mar 5, 2008 · 22 comments
Closed

setitimer, getitimer wrapper #46493

gpolo mannequin opened this issue Mar 5, 2008 · 22 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@gpolo
Copy link
Mannequin

gpolo mannequin commented Mar 5, 2008

BPO 2240
Nosy @loewis, @birkenfeld, @tpn
Files
  • setitimer_getitimer.diff: complete patch
  • trunk-itimer.txt: patch backported to trunk
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/loewis'
    closed_at = <Date 2009-04-08.10:50:46.036>
    created_at = <Date 2008-03-05.14:42:12.761>
    labels = ['type-feature']
    title = 'setitimer, getitimer wrapper'
    updated_at = <Date 2009-04-08.10:50:46.034>
    user = 'https://bugs.python.org/gpolo'

    bugs.python.org fields:

    activity = <Date 2009-04-08.10:50:46.034>
    actor = 'tleeuwenburg@gmail.com'
    assignee = 'loewis'
    closed = True
    closed_date = <Date 2009-04-08.10:50:46.036>
    closer = 'tleeuwenburg@gmail.com'
    components = []
    creation = <Date 2008-03-05.14:42:12.761>
    creator = 'gpolo'
    dependencies = []
    files = ['9636', '9646']
    hgrepos = []
    issue_num = 2240
    keywords = ['patch']
    message_count = 22.0
    messages = ['63277', '63278', '63293', '63295', '63297', '63306', '63315', '63319', '63320', '63325', '63390', '63397', '63398', '63399', '63437', '64413', '64999', '65002', '65004', '65129', '65144', '85768']
    nosy_count = 6.0
    nosy_names = ['loewis', 'georg.brandl', 'schmir', 'gpolo', 'trent', 'tleeuwenburg@gmail.com']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue2240'
    versions = ['Python 2.6', 'Python 3.0']

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 5, 2008

    Right now Python misses a wrapper for setitimer and getitimer and I
    believe it would be interesting to include them. I'm (almost) sure some
    other people may find it useful too.

    I'm attaching a standalone module, but if it gets to be included in
    Python, I think it would be better to create a patch against signal
    module. Also, its tests are pretty poor.

    Improvements are welcomed.

    @gpolo gpolo mannequin added the type-feature A feature request or enhancement label Mar 5, 2008
    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 5, 2008

    I forgot to remove an unwanted comment from it =)
    Attaching new version.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Mar 5, 2008

    I can see nothing wrong with including setitimer support for systems
    where it is available, and I agree that the signal module would be the
    right place.

    So whoever wants to complete it, feel free to produce a complete patch
    against the trunk.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 5, 2008

    Martin, thanks for supporting the idea.

    I'm attaching a patch. It is against rev 61255, py3k branch.
    It patches configure, configure.in, Modules/signalmodule.c and pyconfig.h.in
    I wasn't sure if I should attach a diff for each file, so they are all
    packed in the same diff. Also, I wasn't sure if I should append the
    "configure" diff, but I did.

    Documentation in "Doc/" and tests aren't done yet, if someone want to
    pick this task, please say it.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 5, 2008

    I noticed that I forgot to change setitimer and getitimer functions from
    itimer_setitimer to signal_setitimer (same for getitimer).

    I'm attaching a patch that should be applied after the previous one to
    do this renaming.

    @birkenfeld
    Copy link
    Member

    Patch review:

    • ItimerError should be signal.ItimerError, not signal.error.
    • It should probably inherit from EnvironmentError or IOError.
    • itimer_retval will leak the new tuple if one of the PyFloat_FromDouble
      fails.
    • Do you have test and doc patches, too?
    • What does the "which" argument mean, is it an arbitrary integer
      identifying the timer?

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 6, 2008

    Thanks for the review, corrections and suggestions Georg.

    For the first three items: I will be working on this later today.

    For the last item: "which" is one of those three new constants added. I
    believe if you pass something else setitimer/getitimer might raise an
    error (not tested).

    I still have to write doc and tests.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 6, 2008

    I'm attaching another patch, this should be applied after the other ones
    have been applied. It fixes what Georg mentioned.
    I have chosen to let ItimerError inherit from IOError, and improved the
    docstring of setitimer.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 6, 2008

    Updated Doc/library/signal.rst, follows a patch.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 6, 2008

    I've done some tests for getitimer/setitimer, diff is attached.

    @birkenfeld
    Copy link
    Member

    Could you please put all changes in one complete patch? It's much easier
    to review that way.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 8, 2008

    Complete patch attached

    @schmir
    Copy link
    Mannequin

    schmir mannequin commented Mar 8, 2008

    I'd like to also see this in 2.6. Should I update the patch (if it
    doesn't apply) and test? (I guess the signal module hasn't changed that
    much).

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Mar 8, 2008

    If you are going to backport it to 2.6, then the C wrapper should be
    adapted to match Python 2.x C coding style. If the other parts don't
    apply correctly, then you should update it aswell.

    @schmir
    Copy link
    Mannequin

    schmir mannequin commented Mar 10, 2008

    Okay, the patch applies just fine (besides configure, which must be
    regenerated). Running the tests however consumes 3 gb of memory.
    The range functions must be changed to xrange on trunk. I've also
    changed the docs a bit ("new in 2.6").
    I didn't change anything related to coding standards (is this an issue
    with tabs vs spaces?).
    all tests in test_signal work with a patched revision 61332 of trunk, on
    a 64 bit linux.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Mar 24, 2008

    Thanks for the patch. Committed as r61847 and r61848

    @loewis loewis mannequin closed this as completed Mar 24, 2008
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 5, 2008

    Reopening. Apparently, the tests fail on FreeBSD; see

    http://www.python.org/dev/buildbot/trunk/x86%20FreeBSD%203%20trunk/builds/77/step-test/0

    Can you please look into this?

    @loewis loewis mannequin reopened this Apr 5, 2008
    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Apr 5, 2008

    I'm investigating the problem loewis, thanks for reporting. But it would
    be better if someone with running FreeBSD could help me there, in case I
    find the cause for this.

    Also some changes were made to the original patch:

    neal.norwitz did a commit where he says:
    "Using a negative time causes Linux to treat it as zero, so disable that
    test."
    That is not what I get here, maybe a very different kernel, anyway, I
    believe he could have mentioned this here.

    jeffrey.yasskin said:
    ".. fix some flakiness in test_itimer_prof, which could detect that the
    timer had reached 0 before the signal arrived announcing that fact."

    followed by these changes:

    signal.setitimer(self.itimer, 0.2) (old)
    signal.setitimer(self.itimer, 0.2, 0.2) (new) -> not sure the reason
    for this change

    and added:
    self.assertEquals(signal.getitimer(self.itimer), (0.0, 0.0)) -> this is
    the same test I did for itimer_virtual, and it is a bit questionable it
    is really useful at all. I don't understand how these changes matches
    what he comments on his commit.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Apr 5, 2008

    FreeBSD's man page for setitimer/getitimer doesn't look different from
    the one in Linux.
    But.. both tests assumes the computer is not so fast that it finishes
    "for i in xrange(100000000)" before the timer expire, maybe it was not
    the case in that machine. Both timers (virtual and prof) are set to
    expire after 0.2 seconds, and for itimer_virtual it restarts the timer 3
    times before setting it to 0.
    If someone has a freebsd machine (a fast one apparently) that could run
    some tests and help me on this, it would be very nice.

    Also sorry for various typos in my last message.

    @gpolo
    Copy link
    Mannequin Author

    gpolo mannequin commented Apr 7, 2008

    Trent Nelson kindly gave me access to his FreeBSD 6.2 buildbot so I had
    chance to do some tests. The problem happens when Python is built
    against or libc_r, or if you are using libmap you won't need to
    recompile but the problem still happens when using libc_r.

    I started searching in the FreeBSD bug tracker and found this issue:
    http://www.freebsd.org/cgi/query-pr.cgi?pr=threads/49087 which seems
    very similar to the problem related here.

    I've also done a very simple "test" in C, just to demonstrate that this
    issue isn't related to Python at all:

    #include <stdio.h>
    #include <signal.h>
    #include <sys/time.h>
    
    void h(int signo)
    {
        struct itimerval t;
    
        getitimer(ITIMER_PROF, &t);
        printf("%d %d\n", t.it_value.tv_sec, t.it_value.tv_usec);
    
        printf("deactive ITIMER_PROF\n");
        t.it_value.tv_sec = 0;
        t.it_value.tv_usec = 0;
        setitimer(ITIMER_PROF, &t, &t);
    }
    
    int main(void)
    {
        struct itimerval ival;
        ival.it_value.tv_sec = 1;
        ival.it_value.tv_usec = 0;
        ival.it_interval.tv_sec = 1;
        ival.it_interval.tv_usec = 0;
        signal(SIGPROF, h);
        printf("%d\n", setitimer(ITIMER_PROF, &ival, NULL));
        alarm(2);
    
        while (1) {
            getitimer(ITIMER_PROF, &ival);
            if (ival.it_value.tv_sec == 0 && ival.it_value.tv_usec == 0)
                break;
            }
    
        return 0;
    }

    When I compile this using -lc_r then the callback "h" is never called
    and then the alarm is fired. Compiling against pthread, thr or nothing
    (since this example doesn't need any threading library) doesn't
    demonstrate this problem and all is fine (callback "h" is invoked,
    infinite loop finishes and test returns 0).

    Should further discussion be moved to python-dev ? I'm somewhat stuck on
    how to resolve this, besides saying to upgrade to FreeBSD 7 which uses
    libthr by default.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 8, 2008

    Should further discussion be moved to python-dev ? I'm somewhat stuck on
    how to resolve this, besides saying to upgrade to FreeBSD 7 which uses
    libthr by default.

    Discussing on python-dev is fine. An acceptable solution would be to
    omit setitimer/getitimer on FreeBSD if the FreeBSD version is "wrong",
    either at configure time or at compile time. An even better solution
    would be to test at configure time whether these functions work, and
    refuse to expose them if they don't (not sure whether that's possible,
    though, as I don't know when the choice of threading library is made).

    @tleeuwenburggmailcom
    Copy link
    Mannequin

    tleeuwenburggmailcom mannequin commented Apr 8, 2009

    Closing this issue as the main functionality is committed. See issue
    5722 for follow-up regarding FreeBSD functionality.

    @tleeuwenburggmailcom tleeuwenburggmailcom mannequin closed this as completed Apr 8, 2009
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant