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] [FrameworkBundle] Added `semaphore` transport. #34413

Open
wants to merge 19 commits into
base: master
from

Conversation

@CedrickOka
Copy link
Contributor

CedrickOka commented Nov 16, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets any
License MIT
Doc PR any

Semaphore Transport

The Semaphore Transport allows you to send and receive messages on System V message queues. The semaphore transport configuration looks like this :

# .env
MESSENGER_TRANSPORT_DSN=semaphore://%kernel.project_dir%/.env
# Full DSN Example
MESSENGER_TRANSPORT_DSN=semaphore://%kernel.project_dir%/.env?project=M&message_type=1&message_max_size=1024

A number of options can be configured via the DSN or via the options key under the transport in messenger.yaml:

Option Description Default
path Pathname to create System V IPC key
project Project identifier to create System V IPC key M
message_type The type of message to send 1
message_max_size The maximum size in bytes of a message if the message is larger than this size, an exception will be thrown 131072
auto_setup Enable or not the auto-setup of queue true

This extension is not available on Windows platforms.

@CedrickOka CedrickOka requested a review from sroze as a code owner Nov 16, 2019
@CedrickOka CedrickOka force-pushed the CedrickOka:messenger-transport-semaphore branch 5 times, most recently from 8a43137 to b0e7f46 Nov 16, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Nov 16, 2019
@CedrickOka CedrickOka force-pushed the CedrickOka:messenger-transport-semaphore branch 4 times, most recently from 480e8ba to 6179ba5 Nov 16, 2019
@CedrickOka CedrickOka force-pushed the CedrickOka:messenger-transport-semaphore branch from 6179ba5 to 9456873 Nov 19, 2019
@CedrickOka CedrickOka requested a review from ogizanagi Nov 19, 2019
@CedrickOka

This comment has been minimized.

Copy link
Contributor Author

CedrickOka commented Dec 6, 2019

Hi,

@ogizanagi I applied your recommendations of your reviews but I have more news since.
I hope you are well.

Copy link
Member

ogizanagi left a comment

Code looks good and the feature promising to me so far, but I can't play with it for now.

@CedrickOka CedrickOka force-pushed the CedrickOka:messenger-transport-semaphore branch from 9456873 to 4c1375f Dec 9, 2019
@CedrickOka CedrickOka requested a review from ogizanagi Dec 9, 2019
@CedrickOka

This comment has been minimized.

Copy link
Contributor Author

CedrickOka commented Dec 9, 2019

Hi @ogizanagi,

I applied your recommendations, thanks.

@CedrickOka

This comment has been minimized.

Copy link
Contributor Author

CedrickOka commented Dec 23, 2019

Hi @ogizanagi,

I hope you're doing well, I'm still waiting for your reviews

@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Dec 23, 2019

Hi @CedrickOka . As you fixed my review, code looks good to me, but I still need to find some time to give it a try (as anyone else could actually).

@CedrickOka

This comment has been minimized.

Copy link
Contributor Author

CedrickOka commented Dec 24, 2019

Hi @CedrickOka . As you fixed my review, code looks good to me, but I still need to find some time to give it a try (as anyone else could actually).

Thanks

Copy link
Member

ogizanagi left a comment

So I gave it a try on an existing project. It seems to work great so far.

However, about using MESSENGER_TRANSPORT_DSN=semaphore://%kernel.project_dir%/.env
This means the env var must be resolve using '%env(resolve:MESSENGER_TRANSPORT_DSN)%' in order to get %kernel.project_dir% resolved, which is not the default in the recipe: https://github.com/symfony/recipes/blob/5f8acedd41d584611b2422aa3d4d66f97c70c3f2/symfony/messenger/4.3/config/packages/messenger.yaml#L8

@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Jan 6, 2020

Also, this would now target 5.1.
Could you please rebase on master?

@CedrickOka CedrickOka requested review from lyrixx and xabbuh as code owners Jan 10, 2020
@CedrickOka CedrickOka changed the base branch from 4.4 to master Jan 10, 2020
@CedrickOka CedrickOka force-pushed the CedrickOka:messenger-transport-semaphore branch 2 times, most recently from 6156e7c to a95cf86 Jan 10, 2020
@CedrickOka CedrickOka requested a review from ogizanagi Jan 10, 2020
CedrickOka and others added 19 commits Nov 16, 2019
* Fixed too much indent.
* Removed `PlatformUtil` internal class.
* Used method `extension_loaded(sysvmsg)` instead `PlatformUtil::isWindows()`.
@CedrickOka CedrickOka force-pushed the CedrickOka:messenger-transport-semaphore branch from 81372c3 to 89ed51c Jan 14, 2020
@CedrickOka CedrickOka requested a review from lyrixx Jan 14, 2020
@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Jan 19, 2020

Thank you for this PR. I see that you done a great job with this, and this transport looks really good.

However, I am strongly against adding new transports. Symfony should only provide transports that are use by the vast majority of people. AMQP, Redis, Doctrine, and custom transports like sync:// and in-memory:// is enough to cover most applications using messenger.

I know there are special and super cool transports, but I believe that they should not live inside of Symfony organisation.

I vote for closing this PR. And I strongly recommend @CedrickOka to create a library of this great code so other can enjoy this semaphore transport.

@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Jan 26, 2020

For the record:
After discussions in the core team I've changed my mind. Transports like this should be in a separate package in the Symfony organization. Ie, not symfony/messager, but symfony/semaphore-messenger.

The PR will be merged if Semaphore is considered a popular transport.

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