Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Messenger] Pass envelope to message handler as second __invoke parameter#42005

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
adioe3 wants to merge3 commits intosymfony:5.4fromadioe3:envelope-in-handler

Conversation

@adioe3
Copy link

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT

When calling message handlers__invoke() method, pass in the envelope as a second parameter. This ensures we're backwards compatible (doesn't break existing handlers) but any which might want to use the envelope can do so.

My use case for this is a trace ID we want to use for tracking requests across events with AMQP:

  • HTTP request containsX-Trace-Id, controller receives this and dispatches a message with anx-trace-id AMQP header
  • worker collects message, hands off to handler
  • handler does some work and then dispatches another message but it should preserve thex-trace-id header

sstok reacted with thumbs up emojicodedge reacted with hooray emoji
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@ro0NL
Copy link
Contributor

it should preserve the x-trace-id header

shouldnt you solve this more generically at the middleware layer? or make it part of your message payload?

@chalasrchalasr added this to the5.4 milestoneJul 6, 2021
@adioe3
Copy link
Author

adioe3 commentedJul 6, 2021
edited
Loading

it should preserve the x-trace-id header

shouldnt you solve this more generically at the middleware layer? or make it part of your message payload?

It is impossible: the handler will dispatch another message and there is currently no way to get the envelope stamps in the handler. It is also impossible to reference a previous message from another dispatched message without some very ugly solutions like keeping a static map ofspl_object_id()s or something in before/after middlewares which is gnarly.

And adding it to the message body is also bad because the trace ID is for debugging/monitoring purposes and has no value for the core functionality of the respective handlers.

That said, I am all ears for alternative solutions. Explained in code, this is what a handler does:

class SomeHandlerimplements HandlerInterface {privateMessageBusInterface$bus;publicfunction__construct(MessageBusInterface$bus) {$this->bus =$bus;  }publicfunction__invoke(SomeMessage$message) {// do something with $message$this->bus->dispatch(newAnotherMessage(...));  }}

I've implemented also aTraceIdStamp and I have aTraceIdMiddleware which will stamp outgoing messages (add the AMQP header) so the aboveinvoke() would actually do

$this->bus->dispatch(  Envelope::wrap(newAnotherMessage(...))    ->with(newTraceIdStamp($traceId)));

@Nyholm
Copy link
Member

I think you can solve your problem by just type hint for an envelope in your handler.

Ie:

class FooHandler {publicfunction__invoke(Envelope$envelope) {$message->getMessage();// Do work// Have access to all stamps and your X-Trace-Id  }}

Of course, with a handler like this you cannot rely on autoconfigure, you need to write DI config yourself to map a message to a handler.

Koc reacted with thumbs up emoji

@adioe3
Copy link
Author

This doesn't work sinceHandleMessageMiddleware will try to pass-in the message instead of the envelope (i.e. we're not autoconfiguring/wiring anything here):

publicfunction handle(Envelope$envelope,StackInterface$stack):Envelope    {$handler =null;$message =$envelope->getMessage();//      ...try {$handler =$handlerDescriptor->getHandler();$handledStamp = HandledStamp::fromDescriptor($handlerDescriptor,$handler($message));

So even if I manually wire a handler to consume my message class it's still not getting the envelope since the handlers are not handled as services during__invoke() calls.

@adioe3
Copy link
Author

I mean I can write my own handling middleware and jam it before the default one but that seems like extreme overkill.

@ro0NL
Copy link
Contributor

ro0NL commentedAug 9, 2021
edited
Loading

It is also impossible to reference a previous message from another dispatched message without some very ugly solutions like keeping a static map of spl_object_id()s or something in before/after middlewares which is gnarly.

IIUC a middleware could track state yes, something like

handle($envelope,$stack) {if ($this->currentTrace) {$envelope->with(newBadge($this->currentTrace))return$stack()->next()->handle($envelope);  }$this->currentTrace =$envelope->last(Badge::class);try {return$stack()->next()->handle($envelope);  }finally {$this->currentTrace =null; }}

note im not particularly against providing envelope as second arg, other than maybe-semantics.

@adioe3
Copy link
Author

@ro0NL I've tried that as well but it's not what I need -- that would tag anything coming down a specific middleware and I need messages from a specific request tied together. Practically speaking I need this:

  • Message A withx-trace-id: 1 is handled in handler which dispatches Message B and slaps onx-trace-id: 1
  • unrelated Message C is handled as usual

In the above stateful-middleware approach what happens is:

  • Message A'sx-trace-id: 1 is stored in middleware
  • Message B dispatched from A's handler gets thex-trace-id slapped on
  • unrelated message C gets thex-trace-id slapped on which is wrong since it's not related to A or B

So far the only two solutions (aside from adding the second invoke param) I see are either:

  1. writing my ownHandleMessageMiddleware that does what I want (99% copy/paste of the current) and slaps on aHandledStamp
  2. writing some kind of stateful middleware which keeps track of incoming and outgoing messages and tacks IDs on (very shaky, limits me to a single consumer process without some sort of shared cache, overall horrible solution as well)

Adding the second invoke param still feels like the best solution and semantically I don't think it's that big of an issue. If we're copying the concept or IRL mail with envelopes, you will usually have the envelope together with the mail, right?

@ro0NL
Copy link
Contributor

ro0NL commentedAug 11, 2021
edited
Loading

im not sure i understand. Who's dispatching message C then?

As i currently see it:
controller invokes => dispatch message A with trace => dispatch message B (receives trace from most outer dispatch)
?? => dispatch message C (handled as usual)

@adioe3
Copy link
Author

adioe3 commentedAug 11, 2021
edited
Loading

There is no controller (and the cake is a lie) -- this is all happening inside abin/console messenger:consume loop.

The worker's job is simple: in a loop it will:

  1. fetch a message from bus/transport,
  2. run it through all the middlewares which will stamp various stamps on it,
  3. either consider it handled or send it if there's noReceivedStamp

Now, say some external system dispatches message A withx-trace-id: 1.

The worker will:

Loop 1

  1. fetch message A from RabbitMQ
  2. it hitsTraceMiddleware which savesx-trace-id: 1 internally
  3. it hitsSomeHandler which will inside its__invoke() method dispatch message B
  4. mark message A as handled

Loop 2

  1. fetch newly dispatched message B
  2. it hitsTraceMiddleware which hasx-trace-id: 1 so it will stamp message B withx-trace-id: 1
  3. continues as normal and marks message B as handled

Loop 3

  1. fetch message C which can beany other message and it has nox-trace-id header set
  2. it hitsTraceMiddleware which hasx-trace-id: 1 so it will erroneously stamp message C withx-trace-id: 1
  3. continues as normal and marks message C as handled

Basically, after one message with a trace ID any future messages without a trace ID will get stamped which is wrong. In the three loops above, messages A and B are related and should have the same trace ID but message C is not.

In other words, a stateful middleware relies on the idea that messages dispatched in a handler will be processed immediately after (in a series) and that is not a good guarantee. I imagine running multiple workers would be problematic, synchronizing thex-trace-id across the workers'TraceMiddleware also.

None of these problems exist when the envelope is available in the handler.

@ro0NL
Copy link
Contributor

ro0NL commentedAug 11, 2021
edited
Loading

im still trying to follow (sorry, i have no practical experience, but this is on my list to play with for logging purposes :)). However i dont like the idea of dealling with trace IDs at the handler level personally :/

with the approach from#42005 (comment)

In loop 1, after message A leaves we clear the state (ie. the finally block)
In loop 2, message B is handled with its own trace ID set, as previous obtained during dispatch from message A
In loop 3, message C is handled as usual given the state was already cleared internally

@ro0NL
Copy link
Contributor

in real life, if you are a stamp collector, you need the envelop. Hence im not really against this. But in real life also we dont usualy link the envelop with the message once opened, which is a sementical concern.

@adioe3
Copy link
Author

I think it would be easiest to try and write a solution so you can see what I mean. Just take any AMQP-compatible queue (I'm using RabbitMQ) and try to pass a custom header from one message to another that you dispatch.

I also need this for logging purposes so that we can identify when in a chain of events something went wrong but we have some handlers which dispatch other messages and, well, here we are.

Regarding the real life example, a couple alternative ones would be:

  • if I get a death threat in the mail, I'm going to contact the police with the sender's address from the envelope. I can't know it's a death threat until I've read the letter and therefore will not dispose of the envelope until I've "handled" the message
  • another would be incoming donations: for each donation I take the senders address and write back a custom "thank you" note using info from the envelope

Semantically, I think we're not losing much...

ro0NL, codedge, and dorrogeray reacted with thumbs up emoji

@adioe3
Copy link
Author

@sroze when could we hope for a review/decision?

@okwinza
Copy link
Contributor

Looks like#42873 also fixes this. So this could be closed when that one is merged.

adioe3 reacted with thumbs up emoji

@adioe3
Copy link
Author

Turns out the approach from@ro0NL works: any dispatches inside a handler are executed immediately and middleware instances are bus-specific so doing a stateful middleware which wipes the IDs post-handling works. I'm closing this as my problem is resolved and the use case is gone. Cheers!

@adioe3adioe3 closed thisSep 21, 2021
@adioe3adioe3 deleted the envelope-in-handler branchOctober 6, 2021 09:36
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

7 participants

@adioe3@carsonbot@ro0NL@Nyholm@okwinza@chalasr@derrabus

[8]ページ先頭

©2009-2025 Movatter.jp