Lucene - Core
  1. Lucene - Core
  2. LUCENE-6075

SimpleRateLimiter cast overflow results in Thread.sleep exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
        Michael McCandless added a comment -

        +1, I'll fix this.

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

        Simple patch w/ test.

        Show
        Michael McCandless added a comment - Simple patch w/ test.
        Hide
        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
        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
        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
        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
        Michael McCandless added a comment -

        Thanks Boaz Leskes!

        Show
        Michael McCandless added a comment - Thanks Boaz Leskes !
        Hide
        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
        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
        Robert Muir added a comment -

        reopen for backport

        Show
        Robert Muir added a comment - reopen for backport
        Hide
        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
        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
        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
        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
        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
        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
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development