fix infinite loop with empty strategy list #2

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@kamilsk

kamilsk commented Jul 18, 2016

Ready to write test if @Rican7 not against the patch

TODO:

  • fix broken tests
@kamilsk

This comment has been minimized.

Show comment Hide comment
@kamilsk

kamilsk Jul 18, 2016

related with #1

kamilsk commented Jul 18, 2016

related with #1

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Jul 18, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 669c09b on kamilsk:patch-1 into 272ad12 on Rican7:master.

coveralls commented Jul 18, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 669c09b on kamilsk:patch-1 into 272ad12 on Rican7:master.

@Rican7

This comment has been minimized.

Show comment Hide comment
@Rican7

Rican7 Jul 18, 2016

Owner

This changes the retry.Retry function to only make one attempt if the strategies are empty, right? The strategies are supposed to be optional, and this makes the library no longer "retry" the action. In this case, it's just performing the action once. That doesn't really jive with the idea of the library. Does that make sense?

Owner

Rican7 commented Jul 18, 2016

This changes the retry.Retry function to only make one attempt if the strategies are empty, right? The strategies are supposed to be optional, and this makes the library no longer "retry" the action. In this case, it's just performing the action once. That doesn't really jive with the idea of the library. Does that make sense?

@kamilsk

This comment has been minimized.

Show comment Hide comment
@kamilsk

kamilsk Jul 18, 2016

func TestInfiniteLoop(t *testing.T) {
    ts := httptest.NewServer(http.HandlerFunc(func (rw http.ResponseWriter, req *http.Request) {
        // expected bad response for this test case
        rw.WriteHeader(http.StatusInternalServerError)
        rw.Write([]byte("Internal Server Error"))
    }))

    // this is simplified internal action with some logic for our business app
    action := func(attempt uint) error {
        resp, err := http.Get(ts.URL)
        if err != nil {
            return err
        }
        if resp.StatusCode != 200 {
            return errors.New("bad response")
        }
        return nil
    }

    // this is simplified configuration, our app is configured from YAML
    cfg := struct {
        Strategies []strategy.Strategy
    }{}

    // we integrate retry library, but our tests with expected bad response are broken...
    Retry(action, cfg.Strategies...)
}

kamilsk commented Jul 18, 2016

func TestInfiniteLoop(t *testing.T) {
    ts := httptest.NewServer(http.HandlerFunc(func (rw http.ResponseWriter, req *http.Request) {
        // expected bad response for this test case
        rw.WriteHeader(http.StatusInternalServerError)
        rw.Write([]byte("Internal Server Error"))
    }))

    // this is simplified internal action with some logic for our business app
    action := func(attempt uint) error {
        resp, err := http.Get(ts.URL)
        if err != nil {
            return err
        }
        if resp.StatusCode != 200 {
            return errors.New("bad response")
        }
        return nil
    }

    // this is simplified configuration, our app is configured from YAML
    cfg := struct {
        Strategies []strategy.Strategy
    }{}

    // we integrate retry library, but our tests with expected bad response are broken...
    Retry(action, cfg.Strategies...)
}
@kamilsk

This comment has been minimized.

Show comment Hide comment
@kamilsk

kamilsk Jul 18, 2016

we have added check for empty cfg.Strategies (now it's so). if this is the right way, it's ok - I will remove PR and close the issue

kamilsk commented Jul 18, 2016

we have added check for empty cfg.Strategies (now it's so). if this is the right way, it's ok - I will remove PR and close the issue

@Rican7

This comment has been minimized.

Show comment Hide comment
@Rican7

Rican7 Jul 18, 2016

Owner

Yea, see the issue is that your test is invalid, or rather is incorrectly testing an expected condition.

If you expect the retry.Action to fail and never succeed, then you should have your test limit the number of retries (or just not pass to retry at all).

Thanks for understanding. Please don't hesitate to let me know if I may be misunderstanding something. 😃

Owner

Rican7 commented Jul 18, 2016

Yea, see the issue is that your test is invalid, or rather is incorrectly testing an expected condition.

If you expect the retry.Action to fail and never succeed, then you should have your test limit the number of retries (or just not pass to retry at all).

Thanks for understanding. Please don't hesitate to let me know if I may be misunderstanding something. 😃

@kamilsk

This comment has been minimized.

Show comment Hide comment
@kamilsk

kamilsk Jul 18, 2016

Please don't hesitate to let me know if I may be misunderstanding something.

  1. I have the "action" with complex logic inside (run RPC, handle header, react for response code, etc.)
  2. I have tests for that action where it handle some "bad cases" (if remote server returns some bad header, or something else) - many different cases covered by tests
  3. I wrap the "action" by retry.Retry
  4. I provide for retry.Retry strategies from configuration storage (etcd, yaml, etc.)
  5. []strategy.Strategies for old tests is empty, because nil is correct value for slice
  6. Tests with bad cases are broken, because retry.Retry with empty slice run action infinite times

How I can fix that:

  1. change all tests and add "default" strategy for all of them
  2. handle config (showed you inside issue #1)
    if len(cfg.RetryStrategies) == 0 {
        result.retryStrategies = []strategy.Strategy{
            strategy.Limit(0),
        }
    } else {
        result.retryStrategies = cfg.RetryStrategies
    }
  1. create patch for retry library
    The problem is that the empty slice is correct value: Retry(action, []strategy.Strategy{}...)
  2. handle inside call action
if len(strategies) > 0 {
    retry.Retry(action, strategies...)
} else {
    action()
}

:(

kamilsk commented Jul 18, 2016

Please don't hesitate to let me know if I may be misunderstanding something.

  1. I have the "action" with complex logic inside (run RPC, handle header, react for response code, etc.)
  2. I have tests for that action where it handle some "bad cases" (if remote server returns some bad header, or something else) - many different cases covered by tests
  3. I wrap the "action" by retry.Retry
  4. I provide for retry.Retry strategies from configuration storage (etcd, yaml, etc.)
  5. []strategy.Strategies for old tests is empty, because nil is correct value for slice
  6. Tests with bad cases are broken, because retry.Retry with empty slice run action infinite times

How I can fix that:

  1. change all tests and add "default" strategy for all of them
  2. handle config (showed you inside issue #1)
    if len(cfg.RetryStrategies) == 0 {
        result.retryStrategies = []strategy.Strategy{
            strategy.Limit(0),
        }
    } else {
        result.retryStrategies = cfg.RetryStrategies
    }
  1. create patch for retry library
    The problem is that the empty slice is correct value: Retry(action, []strategy.Strategy{}...)
  2. handle inside call action
if len(strategies) > 0 {
    retry.Retry(action, strategies...)
} else {
    action()
}

:(

@kamilsk

This comment has been minimized.

Show comment Hide comment
@kamilsk

kamilsk Jul 18, 2016

Just imagine that the strategy list is configurable from outside the project. From etcd, json, yaml, etc. It can be different for many cases in runtime (if graceful degradation enabled or balancer decrease upstreams count).

kamilsk commented Jul 18, 2016

Just imagine that the strategy list is configurable from outside the project. From etcd, json, yaml, etc. It can be different for many cases in runtime (if graceful degradation enabled or balancer decrease upstreams count).

@Rican7

This comment has been minimized.

Show comment Hide comment
@Rican7

Rican7 Jul 18, 2016

Owner

How I can fix that:

  1. change all tests and add "default" strategy for all of them
  2. handle config (showed you inside issue #1)

It depends on what you're actually trying to test vs what you actually want your code to do.

Do you want your application (not tests) code to infinitely retry? In some service cases that may make sense, but in others you may want to limit the retries to a certain amount.

If you don't want your application code to infinitely retry, then simply limit it by passing a strategy.Limit to the retry.Retry call. If you want to pass other strategies dynamically, then simply have the strategy.Limit "prepend" a list of strategies that are dynamically passed to the caller.

If you do want your application code to infinitely retry, then your tests that test an error condition should work around the application's need to actually behave in that manner of retying infinitely or a number of times. This can be achieved via a "default" of some sort with an override, much like the configuration that you previously mentioned. If your test doesn't doesn't work around the actions need to retry, then it's not actually testing the behavior correctly. Your tests shouldn't test the behaviors of this retry library, but they should test that the action is actually being performed by the retry loop. 😃

Owner

Rican7 commented Jul 18, 2016

How I can fix that:

  1. change all tests and add "default" strategy for all of them
  2. handle config (showed you inside issue #1)

It depends on what you're actually trying to test vs what you actually want your code to do.

Do you want your application (not tests) code to infinitely retry? In some service cases that may make sense, but in others you may want to limit the retries to a certain amount.

If you don't want your application code to infinitely retry, then simply limit it by passing a strategy.Limit to the retry.Retry call. If you want to pass other strategies dynamically, then simply have the strategy.Limit "prepend" a list of strategies that are dynamically passed to the caller.

If you do want your application code to infinitely retry, then your tests that test an error condition should work around the application's need to actually behave in that manner of retying infinitely or a number of times. This can be achieved via a "default" of some sort with an override, much like the configuration that you previously mentioned. If your test doesn't doesn't work around the actions need to retry, then it's not actually testing the behavior correctly. Your tests shouldn't test the behaviors of this retry library, but they should test that the action is actually being performed by the retry loop. 😃

@kamilsk kamilsk closed this Jul 18, 2016

@Rican7

This comment has been minimized.

Show comment Hide comment
@Rican7

Rican7 Jul 18, 2016

Owner

Sorry for any confusion. Thanks for taking the time to work this out with me. 😃

Owner

Rican7 commented Jul 18, 2016

Sorry for any confusion. Thanks for taking the time to work this out with me. 😃

@kamilsk kamilsk changed the title from [WIP] fix infinite loop with empty strategy list to fix infinite loop with empty strategy list Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment