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

Pushing new items to the queue while it’s already processing #7

Closed
crshng opened this issue Mar 31, 2016 · 8 comments · Fixed by #97
Closed

Pushing new items to the queue while it’s already processing #7

crshng opened this issue Mar 31, 2016 · 8 comments · Fixed by #97

Comments

@crshng
Copy link

crshng commented Mar 31, 2016

What's the proper way to add additional items to a processing queue? I'm a little confused as to when when to save and dispatch the background processing queue. Could you please provide an example/pattern for doing this?

@No3x
Copy link
Contributor

No3x commented Aug 14, 2016

I was about to ask the same.
The readme says:

Queues work on a first in first out basis, which allows additional items to be pushed to the queue even if it’s already processing.

I'm basically doing this:

for ($i=0; $i<3; $i++) {
    $itemToAdd = ['title' => "test #$i"];
    $this->asyncWorker->push_to_queue($itemToAdd);
    $this->asyncWorker->save();
    $this->asyncWorker->dispatch();
}

Save and dispatch are not outside the loop as this simulates the asynchronous action to put items to the queue.

But I think save is wrong as the job is already dispatched and running. save would write the data to the db even if it was already processed.
The log illustrates this:

Async.DEBUG: Processing item: test #0 
Async.INFO: Queue is empty. Waiting for the next items...
Async.DEBUG: Processing item: test #0 
Async.DEBUG: Processing item: test #1 
Async.INFO: Queue is empty. Waiting for the next items...
Async.DEBUG: Processing item: test #0 
Async.DEBUG: Processing item: test #1 
Async.DEBUG: Processing item: test #2 
Async.INFO: Queue is empty. Waiting for the next items...

Did you find a solution?

My workaround:

public function save()
{
    parent::save();
    $this->data = [];
    return $this;
}

Empty data at the end of the save method.
save creates a batch that is processed later on by the worker. data is used for the purpose of create a batch only.
Disadvantage: Multiple action_name_batch_ options (with size one) in the database instead of multiple large.
Recommendation: collect chunks of items in data and save them to create batches of lager size. ( Is possible when processing collections, is not possible when asynchronous adding items to the queue (e.g. "events"))

@nosilver4u
Copy link

I fixed this in the copy of the libs I use for https://github.com/nosilver4u/ewww-image-optimizer/
Perhaps I should try to make it into a proper pull request, but I substantially changed the queue behavior, so that you have queue A and queue B instead of tons of queues with 1 item. Whichever queue is not locked has new items added, and there are only ever two queues in existence. If Ashley is interested, I might attempt it. Otherwise, you can just grab them from the vendor folder of EWWW IO.

@ianmjones
Copy link
Member

@No3x's example would be more idiomatic if it was like this ...

for ($i=0; $i<3; $i++) {
    $itemToAdd = ['title' => "test #$i"];
    $this->asyncWorker->push_to_queue($itemToAdd);
}
$this->asyncWorker->save()->dispatch();

This adds multiple items to the queue for a batch, saves the batch, and then dispatches the background process worker to start working on the first batch it finds. If there's already a background process running, the newly dispatched one will bail, and the existing one will eventually get to the newly added batch of queued items.

It's perfectly fine to have batches with single items in their queue, that's often the way people use it, but if you have a chunk of items to be added to the queue in one PHP process, it's also fine to queue them up on a batch before saving it, it'll be somewhat more performant.

If there's a chance that the PHP process may error out before queuing the next item for a batch, probably safest to save and dispatch the batch after queuing each item so that it'll not get lost.

@No3x
Copy link
Contributor

No3x commented Apr 11, 2023

@ianmjones but the question was how to add items to an already processing queue if you don't know the batches from the beginning.

Save and dispatch are not outside the loop as this simulates the asynchronous action to put items to the queue.

@ianmjones
Copy link
Member

but the question was how to add items to an already processing queue if you don't know the batches from the beginning.

I think there's a slight misunderstanding here due to the way we talk about queuing items and saving a batch, and how that relates to an already running background process.

If while a background process is already running you create a new instance of your WP_Backgroud_Process subclass, use push_to_queue() one or more times, and then call save(), those queued items are now ready to be processed by the already running background process.

We only advise always then calling dispatch() even if it is expected that a background process is already running, simply to ensure that there isn't a chance that the queue emptied just before you saved the new batch of items to be processed, and the background process completed. Calling dispatch when there's already a background process running is safe, it'll recognise that there's already an instance running and leave it to carry on processing its just queued items.

One thing you definitely do not want to do is find an existing batch and alter it by adding new items to it. If you can guarantee that the batch is not currently being processed, then you may just get away with it, but there's a good chance that it might be in process and the in-memory version in the background process overwrites your changes when a task() returns.

Does that make sense? Or did I misunderstand the issue being discussed here?

@No3x
Copy link
Contributor

No3x commented Apr 11, 2023

Thanks for the details. I'm sure it helps adaptors of this lib to understand the behaviour a bit better.
Your sample, you called idiomatic, just does not reflect adding to an already running queue (you push to queue and then dispatch) whereas my example focused on this concern. Therefore I do not understand the value regarding this issue. Or should this example not be read as MWE and you assume prior code (a prior dispatch in particular)?

@ianmjones
Copy link
Member

Or should this example not be read as MWE and you assume prior code (a prior dispatch in particular)?

What does "MWE" stand for in this context?

Having another look at your example code, log output, and workaround, I get what you're saying now!

You're hitting a bug whereby the dispatch is being sent an ever-growing data[] set in a tight loop, each time processing all the queued items over again.

Your workaround is virtually the same as what we'll be bringing to the library in the next version, it's a fix we've already got in this library's father, WP Offload Media!

As such, seams like a good idea to reopen this issue and mark it as a bug so you can track the fix when it lands.

Thanks for clarifying the issue @No3x!

@No3x
Copy link
Contributor

No3x commented Apr 11, 2023

Please see this page for the meaning of MWE I'm referring to https://stackoverflow.com/help/minimal-reproducible-example. Unfortunately I'm not into the details of the implementation anymore but I will review and follow this issue.
Thanks for your time @ianmjones

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