Skip to content

[5.8] Only remove the event mutex if it was created#29526

Merged
taylorotwell merged 1 commit intolaravel:5.8from
lorisleiva:5.8
Aug 12, 2019
Merged

[5.8] Only remove the event mutex if it was created#29526
taylorotwell merged 1 commit intolaravel:5.8from
lorisleiva:5.8

Conversation

@lorisleiva
Copy link
Copy Markdown
Contributor

The purpose of this PR is to fix an issue that throws PHP Fatal errors when running the scheduler in a test.

Description of the issue

  • Given a scheduled CallbackEvent with a description (i.e. using the name() method)
  • When we run the scheduler in a test using $this->artisan('schedule:run')
  • Then the test passes but it throws PHP Fatal errors (see image below)

EA-0k1NXsAEY_Qw

Note that, since $schedule->job() is syntactic sugar for $schedule->call()->name(), the same issue occurs when scheduling a job. Please see issue #29394 for more information including the steps to reproduce on a fresh Laravel install.

The origin of the problem

The PHP Fatal errors are thrown when removing the mutex of a CallbackEvent. (see source)

protected function removeMutex()
{
    if ($this->description) {
        $this->mutex->forget($this);
    }
}

As you can see we only remove the mutex if a description was provided. However, since we are not using the withoutOverlapping option, that mutex was never created in the first place. (see source)

if ($this->description && $this->withoutOverlapping &&
    ! $this->mutex->create($this)) {
    return;
}

The solution

Making sure that the conditions of creating and removing an event mutex are identical fixes our issue.

protected function removeMutex()
{
    if ($this->description && $this->withoutOverlapping) {
        $this->mutex->forget($this);
    }
}

Fix #29394

@driesvints driesvints changed the title Only remove the event mutex if it was created [5.8] Only remove the event mutex if it was created Aug 12, 2019
@taylorotwell taylorotwell merged commit 8550ff3 into laravel:5.8 Aug 12, 2019
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.

Scheduling jobs throws error in tests

2 participants