Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-6075

SimpleRateLimiter cast overflow results in Thread.sleep exception

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.10.3, 5.0, 6.0
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      SimpleRateLimiter.pause() uses an uncheck cast of longs to ints:

      Thread.sleep((int) (pauseNS/1000000), (int) (pauseNS % 1000000));

      Although we check that pauseNS is positive, however if it's large enough the cast to int produces a negative value, causing Thread.sleep to throw an exception.

      We should protect for it.

      1. LUCENE-6075.patch
        3 kB
        Michael McCandless

        Activity

        Hide
        mikemccand Michael McCandless added a comment -

        +1, I'll fix this.

        Show
        mikemccand Michael McCandless added a comment - +1, I'll fix this.
        Hide
        mikemccand Michael McCandless added a comment -

        Simple patch w/ test.

        Show
        mikemccand Michael McCandless added a comment - Simple patch w/ test.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1641584 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1641584 ]

        LUCENE-6075: don't overflow int in SimpleRateLimiter

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1641584 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1641584 ] LUCENE-6075 : don't overflow int in SimpleRateLimiter
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1641585 from Michael McCandless in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1641585 ]

        LUCENE-6075: don't overflow int in SimpleRateLimiter

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1641585 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1641585 ] LUCENE-6075 : don't overflow int in SimpleRateLimiter
        Hide
        mikemccand Michael McCandless added a comment -

        Thanks Boaz Leskes!

        Show
        mikemccand Michael McCandless added a comment - Thanks Boaz Leskes !
        Hide
        bleskes Boaz Leskes added a comment -

        Thank you for fixing it.

        I ran into it in code that ran in a VM and I suspect (though can't be sure) it had something to do with virtualized time. I wonder if it makes sense to have a "sanity check" upper bound to rate limiting - as sleeping for 25 days is most likely not the intended behaviour..

        Show
        bleskes Boaz Leskes added a comment - Thank you for fixing it. I ran into it in code that ran in a VM and I suspect (though can't be sure) it had something to do with virtualized time. I wonder if it makes sense to have a "sanity check" upper bound to rate limiting - as sleeping for 25 days is most likely not the intended behaviour..
        Hide
        rcmuir Robert Muir added a comment -

        reopen for backport

        Show
        rcmuir Robert Muir added a comment - reopen for backport
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1642666 from Michael McCandless in branch 'dev/branches/lucene_solr_4_10'
        [ https://svn.apache.org/r1642666 ]

        LUCENE-6075: don't overflow int in SimpleRateLimiter

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1642666 from Michael McCandless in branch 'dev/branches/lucene_solr_4_10' [ https://svn.apache.org/r1642666 ] LUCENE-6075 : don't overflow int in SimpleRateLimiter
        Hide
        mikemccand Michael McCandless added a comment -

        I wonder if it makes sense to have a "sanity check" upper bound to rate limiting - as sleeping for 25 days is most likely not the intended behaviour..

        25 days is clearly rather silly ... but I'm not sure we should add such checks at this low level. Where would you draw the line?

        Show
        mikemccand Michael McCandless added a comment - I wonder if it makes sense to have a "sanity check" upper bound to rate limiting - as sleeping for 25 days is most likely not the intended behaviour.. 25 days is clearly rather silly ... but I'm not sure we should add such checks at this low level. Where would you draw the line?
        Hide
        bleskes Boaz Leskes added a comment -

        Yeah, that's the tricky part which made me wonder (as opposed to a suggestion ) The only thought I had is maintaining a last update time stamp (on top on lastNS) and if that goes backwards, adapt for it? I'm not sure how often this happens in practice though. From the reaction, I judge that it's not really often and we just have stepped into a very rare case. I'm OK with leaving as is. Thx for the fix!

        Show
        bleskes Boaz Leskes added a comment - Yeah, that's the tricky part which made me wonder (as opposed to a suggestion ) The only thought I had is maintaining a last update time stamp (on top on lastNS) and if that goes backwards, adapt for it? I'm not sure how often this happens in practice though. From the reaction, I judge that it's not really often and we just have stepped into a very rare case. I'm OK with leaving as is. Thx for the fix!
        Hide
        anshumg Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        anshumg Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            bleskes Boaz Leskes
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development