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

[Messenger] Add a "in-memory://" transport #29097

Merged
merged 2 commits into from Apr 6, 2019

Conversation

@GaryPEGEOT
Copy link
Contributor

commented Nov 5, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29040
License MIT
Doc PR Todo

Add a new InMemoryTransport for test purpose, usable by starting your DSN by in-memory://

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Just a random thought: Isn't a null transport just a special case of a memory transport without any listener?

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Note regarding #29097 (comment): that would mean making the MemoryTransport MessageBusInterface $bus constructor argument optional or extract dispatch in a dedicated listener.
No strong opinion yet.

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 6, 2018

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@GaryPEGEOT thanks for this contribution! If this is finally merged, please review the list of Messenger issues and list of pending Messenger pull requests to see if there are issues/PRs conflicting with this change. If there are not, please open an issue in https://github.com/symfony/symfony-docs/issues to track this change. You don't have to provide the docs yourself if you don't want, but this would help us track the change. Thanks!

@sroze

sroze approved these changes Mar 19, 2019

Copy link
Member

left a comment

Except these stylish details to be changed, looks good to me 👍

@GaryPEGEOT GaryPEGEOT force-pushed the GaryPEGEOT:feature/null-transporter branch from 52f0a2c to 09512a5 Mar 19, 2019

@stof

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Just a random thought: Isn't a null transport just a special case of a memory transport without any listener?

If you never flush your MemoryTransport, it would leak memory. So using it instead of a NullTransport looks wrong (having to flush something when you expect it to be a blackhole is not intuitive)

*/
public function send(Envelope $envelope): Envelope
{
$this->sent[] = $envelope;

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 20, 2019

Member

As we accumulate envelopes over time, do we need to implement ResetableInterface to flush the envelopes from one request to the next?

This comment has been minimized.

Copy link
@GaryPEGEOT

GaryPEGEOT Mar 23, 2019

Author Contributor

@sroze, @fabpot I've implemented Symfony\Contracts\Service\ResetInterface and I now see that there is an ack an reject method in the Transport interface. Do you think we need to store which message has been ack / nack?

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

having to flush something when you expect it to be a blackhole is not intuitive)

But it's not just a blackhole (null transport might be misleading in this way): this transport is storing the sent envelopes (to be used in tests for instance). So I would have implemented ResetableInterface on the MemoryTransport and just use a different factory registering it.

@sroze

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@GaryPEGEOT can you implement the ResetableInterface and test that the messages are properly removed?

@GaryPEGEOT GaryPEGEOT force-pushed the GaryPEGEOT:feature/null-transporter branch from 1ade719 to 1711e48 Mar 23, 2019

@sroze

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@GaryPEGEOT

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

@sroze I added the getAcknowledged and getRejected methods

*
* @author Gary PEGEOT <garypegeot@gmail.com>
*/
class NullTransport implements TransportInterface, ResetInterface

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Mar 23, 2019

Member

Note implementing ResetInterface isn't enough for this to be properly integrated with the framework and stored envelopes to be cleared from one request to the next. The service definition must be tagged with kernel.reset.

But as the service is built using a factory and might use an env var for the DSN, it may not be possible to add the tag conditionally from the extension (compile time). Instead, you could implement ResetInterface on the factory too and keep track of created services to clear them from here (so only the factory will be tagged with kernel.reset).

*
* @author Gary PEGEOT <garypegeot@gmail.com>
*/
class NullTransportFactoryTest extends \PHPUnit\Framework\TestCase

This comment has been minimized.

Copy link
@onEXHovia

onEXHovia Mar 24, 2019

maybe worth importing TestCase?
use PHPUnit\Framework\TestCase;

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

I agree with @ogizanagi that I would not expect a null transport to store things.

@sroze

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

to avoid duplication with memory listener

What do you mean? 🤔

@GaryPEGEOT

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

to avoid duplication with memory listener

What do you mean? thinking

memory transport, my bad!, both this transport and the one in #28746 store message in memory, so are kind of duplicate. Should this one do nothing instead, ie not storing any message?

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

I think the idea is this: This one SHOULD store in memory, and be called in-memory. And then #28746 could actually use this transport behind the scenes to read the stored messages and send them.

@sroze

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Exactly 👍

@GaryPEGEOT GaryPEGEOT force-pushed the GaryPEGEOT:feature/null-transporter branch from 53de888 to 38e1b51 Mar 31, 2019

@GaryPEGEOT

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

I think the idea is this: This one SHOULD store in memory, and be called in-memory. And then #28746 could actually use this transport behind the scenes to read the stored messages and send them.

Renamed to InMemoryTransport, as well as the dsn

@GaryPEGEOT GaryPEGEOT changed the title [Messenger] Add a "null://" transport [Messenger] Add a "in-memory://" transport Mar 31, 2019

@sroze sroze force-pushed the GaryPEGEOT:feature/null-transporter branch from c1b34d7 to 83267d2 Apr 6, 2019

@sroze

sroze approved these changes Apr 6, 2019

@sroze sroze force-pushed the GaryPEGEOT:feature/null-transporter branch from 83267d2 to 8e01179 Apr 6, 2019

@sroze sroze force-pushed the GaryPEGEOT:feature/null-transporter branch from 8e01179 to 8f8c82e Apr 6, 2019

@fabpot

fabpot approved these changes Apr 6, 2019

use Symfony\Contracts\Service\ResetInterface;
/**
* Transport that stay in memory. Useful for testing purpose.

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 6, 2019

Member

stays

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Thank you @GaryPEGEOT.

@fabpot fabpot merged commit 8f8c82e into symfony:master Apr 6, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 6, 2019

feature #29097 [Messenger] Add a "in-memory://" transport (GaryPEGEOT…
…, sroze)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] Add a "in-memory://" transport

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29040
| License       | MIT
| Doc PR        | Todo

Add a new `InMemoryTransport` for test purpose, usable by starting your DSN by `in-memory://`

Commits
-------

8f8c82e Make the in-memory transport resettable
fe75920 Add a "null://" transport
@Nyholm
Copy link
Member

left a comment

Thank you

#eu-fossa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.