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

[3.0] set default 'tries' to 1 #704

Merged
merged 1 commit into from
Nov 13, 2019
Merged

Conversation

browner12
Copy link
Contributor

trying a Job more than 1 time can have unintended side effects, and for the less experienced developer, defaulting to 3 tries could cause problems.

my opinion would be that we default to 1 try, and allow the developer who understands the considerations for running a Job multiple times to increase this if they like.

Scenario

class PlaceOrder implement ShouldQueue
{
    public $tries = 3;

    public function __construct(array $products)
    {
        $this->products = $products;
    }

    public function handle()
    {
        //create the Order
        $order = new Order;
        $order->product = $this->products;
        $order->save();

        //do something that could generate an Exception
    }
}

If an Exception is generated, the Job will fail and retry. However, the Order has already been created and saved, and it will do so again on the following try, possibly resulting in 3 Orders when there should have only been one.

trying a Job more than 1 time can have unintended side effects, and for the less experienced developer, defaulting to 3 tries could cause problems.

my opinion would be that we default to 1 try, and allow the developer who understands the considerations for running a Job multiple times to increase this if they like.
@piotrgrabowskielastique
Copy link
Contributor

piotrgrabowskielastique commented Nov 13, 2019

This is already the default behavior of laravel queue worker (laravel 6.x+). Job retry should be opt-in instead of opt-out as its easier for new developers to understand whats happening and to debug, so setting default tries to 1 is a good solution.

@taylorotwell taylorotwell merged commit 4eafbab into laravel:3.0 Nov 13, 2019
@browner12
Copy link
Contributor Author

great point @piotrgrabowskielastique. If you're running a normal queue worker, and not Horizon, the default is 1 try. Keeping it consistent is another good reason to switch.

@browner12 browner12 deleted the patch-2 branch November 13, 2019 15:08
@JayBizzle
Copy link
Contributor

So, if we are currently relying on jobs retrying 3 times by default, we now need to specify public $tries = 3; once this change is released. Am i correct?

Perhaps should have gone on the 4.0 branch.

@driesvints
Copy link
Member

@JayBizzle afaik you always publish your horizon config so it should be fixed in your config file.

@JayBizzle
Copy link
Contributor

Ah, yes, I get it now. Thanks 👍

@browner12
Copy link
Contributor Author

correct, this will only apply to new projects publishing their config asset for the first time.

@antimech
Copy link

antimech commented Nov 28, 2019

@browner12

$ php artisan vendor:publish --tag="horizon-config" --force

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants