calc_retry_wait is broken if Integer is used for retry_wait

[sabottenda]

From v0.10.58 or later and v0.12, calc_retry_wait causes an exception if Integer is used for retry_wait.
retry_wait is time type, so it can be Integer value. #finite? is a method for Float class, not for Integer.

[sonots]

👍

[mr-salty]

Hi - sorry abot this - it was due to my PR #502 although it appears my test didn't catch this problem - could you add a test that fails without this change?

Also, I believe a similar problem to the one I was trying to solve remains - even though 'wait' will not overflow if it is an Integer, the +/- 12.5% calculation on line 410 converts it to float, which can produce NaN and lead to an exception on 412.

So, probably the cleanest fix would be to do the 12.5% retry_wait calculation unconditionally and then check finite? or nan? on the result (which is always a float).

[repeatedly]

Converting retry_wait to Float type in configure seems better than wait.is_a?(Float).
retry_wait is smaller so embbeded float is used on 64bit environment.

From mr-salty comment, adding test is best.

[sabottenda]

Converting retry_wait to Float type in configure seems better than wait.is_a?(Float).

thanks, it's better.

Also, I believe a similar problem to the one I was trying to solve remains - even though 'wait' will not overflow if it is an Integer, the +/- 12.5% calculation on line 410 converts it to float, which can produce NaN and lead to an exception on 412.

@mr-salty I can't find the case to produce NaN. Please tell me the case.

Anyway, I reverted calc_retry_wait and just convert to Float in configure. Please check it.

[repeatedly]

Code seems good so I merged this patch for new release.

[mr-salty]

Hi, @sabottenda - sorry i didn't address your comment before merge but it looks ok to me and I also tested it.

test_large_num_retries is the one that produces the error, so I added 'retry_wait 1s' to the config there and verified that it fails without your change and passes with it. Thanks for taking care of this!

[sonots]

Test fails like

===============================================================================
Error: test_calc_retry_wait_with_integer_retry_wait(FluentOutputTest::BufferedOutputTest)
: NoMethodError: undefined method `finite?' for 131072:Fixnum
/Users/seo.naotoshi/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/fluentd-0.12.2/lib/fluent/output.rb:407:in `calc_retry_wait'
test/test_output.rb:87:in `test_calc_retry_wait_with_integer_retry_wait'
     84:       d.instance.retry_limit.times {
     85:         d.instance.instance_eval { @num_errors += 1 }
     86:       }
  => 87:       assert_equal true, d.instance.calc_retry_wait.finite?
     88:     end
     89:
     90:     def test_large_num_retries
===============================================================================
F
===============================================================================
Failure:
test_configure(FluentOutputTest::BufferedOutputTest)
test/test_output.rb:61:in `test_configure'
     58:
     59:       # retry_wait is converted to Float for calc_retry_wait
     60:       d = create_driver(CONFIG + %[retry_wait 1s])
  => 61:       assert_equal Float, d.instance.retry_wait.class
     62:     end
     63:
     64:     def test_calc_retry_wait
<Float> expected but was
<Fixnum>

[repeatedly]

Maybe, it seems your environment problem. Tests are passed on Travis-CI.

https://travis-ci.org/fluent/fluentd/builds/46752070

[sonots]

Ah, got it, passed. Sorry for making worry about this.

[sonots]

cherry-picked to v0.10