@ro0NL there's no error, it behaves as if it was successful.

I think the reason this won't work is because the code inside this function will not be called until the returned Generator is rewound. So if the return value is ignored, your code simply does not run (because your code is actually part of the iterator being returned and ignored).

To me, the bug is that you should not use a generator in a place that does not expect returning an iterator that will be consumed (or a place expecting to trigger an exception synchronously and not later in the returned iterator for that matter)

Oh I get it. Wouldn't dd silently kill the worker?

No, it never gets called.

you should not use a generator in a place that does not expect returning an iterator

Sure, Messenger should then prevent that because here it's behaving as if it all works.

If it detects the handler is a generator: the worker? Instead of calling it, it iterates over it.

the handler is not a generator. it returns a generator.

The handler is generating the value, why would it consume the value it's generating? 😆

nothing consumes the generator

That's the bug report, correct.

How is it a feature? 🤔 If your handler is (returning) a generator, Messenger doesn't execute it at all, and there's currently nothing preventing you from making your handler (return) a generator.

Its executed

No, that's not correct, it would get executed if you iterate on the generator, but currently Messenger doesn't execute your handler's body in the described situation, but reports successful handler.

But in your example you literally show the code didn't get executed? 🤔

Perhaps leverage the generator from the handled stamp.

WDYM here?

Either way, this should be fixed to at least prevent handlers from returning generators because like this it's a knife without a handle.

And its body didn't get executed, which is the summary of this bug report. 😆

I dont see a reason for workers to consume generators

So prevent handlers from returning generators, I have no problem with that. The issue here is nothing is happening but the logs and everything shows everything works as expected, while nothing actually works.

Maybe we should prevent handlers from returning never or iterator.

Generator is valid return value imho

In that case you must consume it for the handler to actually be invoked, no? It's really hard for me to understand your POV here TBH. :)

Well, async messenger does not care about the return value of the handler and ignores it.

When you make a method return a generator, the whole body of the method is part of the iterator being returned. Nothing of it gets executed synchronously when calling your method. That's how yield works in PHP (and in pretty much every language that has an equivalent feature AFAICT).

Note that in the sync case, I don't see what would be iterating over the iterator. I have the impression that Messenger just pass the return value as is to the caller. So it might be some other parts that iterates the generator in the sync case.

I have no issue with either consuming the generator or preventing the generator from being returned. The main issue here is you can shoot yourself in the face here quite easily and there's nothing in Symfony preventing you from doing that, which can mean your messages are being "processed" for months "correctly" until somebody notices they're actually just being no-op'd, you'll not figure it out from the logs.

To be, this looks like a good use case for a static analysis rule here.

But adding a runtime check in the Messenger component looks hard to me: there is a HandledStamp receiving the return value of the handler. Rejecting some return values looks weird.

How would a static analysis tool know whether it's sync or async? For me it's like a user's issue, the person has to know that async values don't get iterated

@cizordj many projects don't use the sync mode at all. Those projects could easily enable a rule ensure that all callables tagged as AsMessageHandler don't return anything. Of course, if your project relies on both modes, things get harder.

This is an interesting discussion. @dkarlovi is very correct when he argues that it is very easy to shoot yourself in the foot.
The counter argument is "this is how PHP works".

The question is really should Symfony help you not to shoot you in the foot here or not?

I am leaning towards what @stof says:

this looks like a good use case for a static analysis rule here.

(I'm not 100% sure I follow the discussion, please forgive me if I make wrong assumptions and ignore the rest of the message if that is the case)

If I'm understanding it correctly, I don't see why message handlers are special and would need special handling (using runtime checks) compared to any other callable argument in the Symfony ecosystem. Wouldn't this be the case for all callables anywhere in the Symfony code? (e.g. event listeners)

To me, that proves that this is not a Messenger bug (nor feature), but rather part of the PHP language (and even because being lazy is the goal of the Generator PHP feature).

Maybe the thing we can research is why sync transport works. I think people are using the sync transport when developing locally, it would be useful if we can reveal the wrong usage of generators when developing locally.

IMO Messenger is different because the handler is called out of process async, it's quite difficult to notice this, no other callable matches this.

also im not sure if we should only auto-consume in worker mode, creating a new point of confusion

That does make sense to me: if your handler is returning a generator AND you're using it both directly and in worker mode (which was unfortunately my case), it stands to reason you're expecting to consume it when invoking it directly., no? Otherwise, why would it be a generator (except by accident).

What confused me here (and IMO is the dangerous part) is it's hard to realize my consuming it in sync mode is why it was even processed, even though I was only consuming it to log out the progress.

In my mind, the processing was

(handling) AND (consuming just to log)

but it was actually

(consuming to handle and log)

Meaning, my logging had the side-effect of actually handling, which is a mind bending since it's totally backwards.

I still believe something should be done in this scenario to avoid this trap.

this should have been a hint nothing would consume it async

That's what I'm saying, I didn't consider me logging out the progress is actually doing the processing

@ro0NL One thing that could be done here is what you've suggested earlier:

1. disallow generators on handler
2. allow them from the stamp

Maybe even having a dedicated stamp for this would clarify the intent?

@ro0NL

Perhaps leverage the generator from the handled stamp.

I'm looking into this issue again and was wondering what was your idea here exactly? Could you give a short example?

Yes, but the key point of this issue is nothing is actually consuming the generator.

I assumed you meant something like passing the generator in a stamp instead as a return value from the handler, which would allow keeping the generator for sync use and the processing would still work for async use.

Right, the question still remains: would it be possible to yield in sync mode and still work in async mode, deprecation or not.

What if the handler returns an Envelope explicitly with the HandledStamp set, would that work? 🤔

That way you could deprecate handlers returning generators themselves, but still have access to it if desired.

If anyone cares, I've solved this by adding a subscriber to Symfony\Component\Messenger\Event\WorkerMessageHandledEvent.

In sync mode, the event is not triggered, my generator is returned to my command, I can consume it as-is.

In async mode, nothing will consume the generator so my handler does it, it just checks if my HandledStamp result is instance of \Generator, if so it just foreaches on it. It works in both cases for me, it might make sense for Messenger to do something like that?

Note: the above approach does not handle rate limiting exceptions correctly since the exceptions get thrown too late. I've tried using a middleware too but that approach doesn't seem possible either. It seems it's currently impossible having your handler be a generator and have it still work correctly in async.

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

Still very relevant.