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 "non sendable" stamps #31471

Merged
merged 1 commit into from May 21, 2019

Conversation

Projects
None yet
6 participants
@weaverryan
Copy link
Member

commented May 10, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31460
License MIT
Doc PR not needed

Fixes a bug where Symfony serialization of the AmqpReceivedStamp sometimes caused problems.

It's still a mystery why the AmqpReceivedStamp caused a segfault sometimes when going through the Symfony serializer or the VarDumper. But, that stamp really didn't need to be sent on redelivery anyways.

I don't love making the removal the responsibility of the serializers, but it didn't work well anywhere else.

Cheers!

@weaverryan weaverryan added this to the 4.3 milestone May 10, 2019

@weaverryan weaverryan requested a review from sroze as a code owner May 10, 2019

@weaverryan weaverryan force-pushed the weaverryan:messenger-non-sendable-stamps branch from 4a027a1 to 42e4158 May 10, 2019

@sroze
Copy link
Member

left a comment

The use-case is definitely there 👍

@weaverryan weaverryan force-pushed the weaverryan:messenger-non-sendable-stamps branch from 42e4158 to 37a43d6 May 13, 2019

Adding a new NonSendableStampInterface to avoid sending certain stamps
Fixes a bug where Symfony serialization of the AmqpReceivedStamp sometimes caused problems.

@weaverryan weaverryan force-pushed the weaverryan:messenger-non-sendable-stamps branch from 37a43d6 to 34e7781 May 13, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Moved implementation to a new Envelope method.

I've only added the new "non-sendable stamp interface" to 3 transport-specific "received" stamps. Generally speaking, I think that stamps should be sent, unless there is a clear reason otherwise. Allowing stamps to be sendable (the default) can make the messages bigger when being retried after failure. But they also allow for a really beautiful "history" as you can look at an Envelope and see exactly what happened to it over time.

Anyways, this is ready to go!

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

This is still ready to go.

@fabpot

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Thank you @weaverryan.

@fabpot fabpot merged commit 34e7781 into symfony:4.3 May 21, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 21, 2019

feature #31471 [Messenger] Add "non sendable" stamps (weaverryan)
This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Add "non sendable" stamps

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31460
| License       | MIT
| Doc PR        | not needed

Fixes a bug where Symfony serialization of the AmqpReceivedStamp sometimes caused problems.

It's still a mystery why the `AmqpReceivedStamp` caused a segfault *sometimes* when going through the Symfony serializer or the `VarDumper`. But, that stamp really didn't need to be sent on redelivery anyways.

I don't love making the removal the responsibility of the serializers, but it didn't work well anywhere else.

Cheers!

Commits
-------

34e7781 Adding a new NonSendableStampInterface to avoid sending certain stamps

@fabpot fabpot referenced this pull request May 22, 2019

Merged

Release v4.3.0-BETA2 #31577

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.