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

[RFC] add middleware to pass the request context through a Messenger transport #30367

Closed
nicolas-grekas opened this issue Feb 24, 2019 · 6 comments
Labels
Feature Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 24, 2019

When a message is sent to a worker via a Messenger transport, we provide no easy way to serialize the context provided by the request. By context, I can think of e.g host, locale, or even user session/token, maybe also request stack.

What about providing a middleware that would do so, eg by appending a stamp on messages that need that context? (another stamp could be used as a flag to ask for the context maybe? - or we could always serialize that context by default?)

It could also be implemented as a transport, with the sender doing the serialization and the receiver restoring the context for the message?

@javiereguiluz javiereguiluz added Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Messenger labels Feb 24, 2019
@lolmx
Copy link
Contributor

lolmx commented Feb 28, 2019

Indeed it would be nice!

I'm not really fan about custom transport just for context serialization, this sounds to me like a 100% middleware job. Also if in one's project there already is a custom transport he wouldn't have any benefit and would still need to handle it by himself.

I like the idea of a RequestContextStamp to access that data, we do not change message, but enrich envelope.

Would we need a flag stamp to trigger context serialization?
Not really, the middleware could check the RequestContextStamp existence with $envelope->all(RequestContextStamp::class), if none do nothing else add another one which would contains the request context. And client would do a $envelope->last(RequestContextStamp::class)->getContext() whenever he needs it.
Flag works well, it's just another though :)

I'm not sure about always serializing the context by default? I would say no, better to serialize it on demand, because even if there are a lot of use cases, I think that's not the majority.

@pounard
Copy link
Contributor

pounard commented Mar 7, 2019

When a message is sent to a worker via a Messenger transport, we provide no easy way to serialize the context provided by the request. By context, I can think of e.g host, locale, or even user session/token, maybe also request stack.

And that's actually a good thing !

To argue a bit, we do use the messenger component, but for running stateless handlers, outside of any context (in our project, that's a feature that handlers are contextless and stateless). Carrying needed context is either the message itself own responsibility, or your database responsibility (for example client configured locale could be stored somewhere, in some kind of user prefs storage). If your message is about sending an email, you should store the local within the message data, in a way that in this example, locale is a first-class parameter for sending the email properly if you need translation.

Adding a magical middleware to serialize then unserialize this context, in my opinion, sounds wrong for many reasons:

  • it's magic ! I don't like magic, even understandable one,
  • it will be hidden and autoconfigured and people will forget how it work, they will just use it without thinking about it. A consequence could be, for example, that after using it transparently for a while, forgetting about how the locale magically happened to be there when running the handler, if somehow for technical reasons, the application changes and email sending handler becomes an external component built using another technology (C#, go, Rust, NodeJS, anything) plugged somewhere with an AMQP in between, how to deal with this magic locale context within an envelope stamp ? The request stamp will be a Symfony proprietary convention, whilst having a property in a JSON or an XML message is much easier to deal with,
  • headers and stamps should not be used for business purpose, they are technical information for transport, not user data,
  • it will potentially oversize your messages, making them less cheap to (un)serialize,
  • the Symfony framework will start using it transparently, for example the future email component, and it will become an implicit hard dependency, even thought the middleware could technically be deactivated, it will not be possible in real life,
  • I really think that helping a developer magically restore a context is actually not helping him/her, it will be in very simple scenario, when you prototype an application, but it will be burden as soon as you need to make it communicate with other applications that could be built with any kind of techno, not only Symfony not even PHP, which is actually one the goals of having a message broker to start with.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 7, 2019

My point is not to add more magic. My point is that calling a handler synchronously will not yield the same result as doing it async, because the container has some state that will leak when handling in sync. The main ones are the locale, the request, the session.
Actually, there are many use cases where the state of these objects is of importance.
It's true that one can do it on their own and store that context in the message directly. Maybe that's fine. Or maybe we'd like to provide tooling to ease these use cases. That's the topic of the RFC.

@pounard
Copy link
Contributor

pounard commented Mar 7, 2019

My point is not to add more magic. My point is that calling a handler synchronously will not yield the same result as doing it async, because the container has some state that will leak when handling in sync.

I guess that the problem is within the container then, it should not have a state in theory. That's an interesting comment thought, I do not see those use case, yet, I didn't happen to experience any of those. Do you have some concrete example where the container state would alter a handler behaviour ?

EDIT: Sorry for the many edits, I write poor english and correct myself a posteriori too much.

@pounard
Copy link
Contributor

pounard commented Mar 7, 2019

It's true that one can do it on their own and store that context in the message directly. Maybe that's fine.

Actually, if you want to write side-effect-less code, your message should bring all its context by itself, maybe a good help for developers would be to have traits that would help creating such value objets (I don't know, maybe a LocaleAwareTrait along with an interface), which would be populated automatically by such middleware. It would still be the developer's responsibility to add the interface and trait over its message implementation explicitly, but along a good documentation, it might be a good start. We actually did solve partially our own problems using value object oriented traits in a real life messenger based project, except our value object trait and interfaces are business oriented, and not context oriented in our case, still it would work without polluting to much the bus, and without adding any new stamp or even any new envelope layer indirection (which could be a way to solve it as well). Solution here would be in the methodology and not within a technically very opinionated solution: along with a default and optional implementation within Symfony core.

@Tobion
Copy link
Member

Tobion commented Nov 6, 2019

My point is that calling a handler synchronously will not yield the same result as doing it async, because the container has some state that will leak when handling in sync. The main ones are the locale, the request, the session.

I agree with @pounard that if your handlers depend on it, the data should be part of the message (either in the body or at least the headers). That should be an explicit decision. I don't think we need to provide a way to add context automatically to messages. And the question would also be which context? The whole request? All environment variables? That seems like a bad practice and I would suggest to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

5 participants