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

Possible wrong instance of JobProcessing event is fired. #16740

Closed
halaei opened this issue Dec 10, 2016 · 5 comments
Closed

Possible wrong instance of JobProcessing event is fired. #16740

halaei opened this issue Dec 10, 2016 · 5 comments
Labels

Comments

@halaei
Copy link
Contributor

halaei commented Dec 10, 2016

Laravel 5.3
SyncQueue::raiseAfterJobEvent() assumes that the connection name is 'sync' when creating a Events\JobProcessing instance. But that is not a true assumption. At that point, we only know that the queue driver is 'sync' and know nothing about the connection name.

@GrahamCampbell
Copy link
Member

Thanks for the report. How do you suggest we fix this please? :)

@halaei
Copy link
Contributor Author

halaei commented Dec 11, 2016

I think it can be fixed by adding a connenctionName property to the constructor. However, I am thinking maybe it is better to remove the whole event firing code in SyncQueue. My guess is nobody uses those events, at least for jobs pushed to sync queue. Removing events from SyncQueue will also make it easy to register listeners for #16739

@themsaid
Copy link
Member

themsaid commented Dec 11, 2016

A change was introduced in this area to fix an issue with logging failed sync requests that run from within a parent job, take a look here: #16681

I'm not sure if there's a use case where you'll need to have multiple sync drivers, that's why we use sync as a name for the connection if the driver is sync.

@halaei
Copy link
Contributor Author

halaei commented Dec 11, 2016

Thanks @themsaid for the info. I agree that this is not a big deal. Here is a listener for JobProcessing event that makes me happy for now :)

class RefreshDBConnections
{
    /**
     * @var ExceptionHandler
     */
    protected $exceptions;

    public function __construct(ExceptionHandler $exceptions)
    {
        $this->exceptions = $exceptions;
    }

    public function handle(JobProcessing $event)
    {
        if ($event->connectionName != 'sync') {
            try {
                while (\DB::transactionLevel() > 0) {
                    \DB::rollBack();
                }
            } catch (Exception $e) {
                \DB::reconnect();
                $this->exceptions->report($e);
            }
        }
    }
}

@halaei
Copy link
Contributor Author

halaei commented Dec 11, 2016

illuminate.queue.looping event is much better!

@halaei halaei closed this as completed Dec 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants