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.

Sign up for GitHub

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

Jump to bottom

Dont allow subscribing same event multiple times in one subscriber #701

Merged
merged 2 commits into from
Feb 21, 2025
Merged

Dont allow subscribing same event multiple times in one subscriber #701

merged 2 commits into from
Feb 21, 2025

Conversation

DanielBadura
Copy link
Member

@DanielBadura DanielBadura commented Feb 19, 2025 •
edited
Loading

This patch here fixes #655. Listening on the same event multiple times in one subscriber is dangerous. This is all described in the issue.

The patch itself does not contain any BC-Break regarding our API but it is a BC-Break behaviour wise. In my opinion this change is important enough to land as a bugfix due to the high risk this may disrupt production systems significantly.

We should also adjust the API in a dedicated PR so that the SubscriberMetadata can not hold anymore a list of methods for a given event.

@DanielBadura DanielBadura added bug Something isn't working BC-Break labels Feb 19, 2025
@DanielBadura DanielBadura self-assigned this Feb 19, 2025
@DanielBadura DanielBadura changed the base branch from 3.11.x to 3.10.x February 19, 2025 12:41
@github-actions GitHub Actions
Copy link

Hello 👋

here is the most recent benchmark result: 

SimpleSetupStreamStoreBench
===========================

+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
|                                        | time (kde mode)                                     | memory                                     |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| subject                                | Tag: <current>     | Tag: base          | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| benchLoad1Event ()                     | 981.400μs (±0.00%) | 995.400μs (±0.00%) | -1.41%    | 34.382mb        | 34.913mb   | -1.52%      |
| benchLoad10000Events ()                | 55.561ms (±0.00%)  | 56.595ms (±0.00%)  | -1.83%    | 34.382mb        | 34.382mb   | 0.00%       |
| benchSave1Event ()                     | 1.098ms (±0.00%)   | 1.312ms (±0.00%)   | -16.28%   | 34.382mb        | 34.382mb   | 0.00%       |
| benchSave10000Events ()                | 293.832ms (±0.00%) | 302.601ms (±0.00%) | -2.90%    | 34.382mb        | 34.382mb   | 0.00%       |
| benchSave10000Aggregates ()            | 9.262s (±0.00%)    | 9.257s (±0.00%)    | +0.06%    | 34.382mb        | 34.382mb   | 0.00%       |
| benchSave10000AggregatesTransaction () | 5.098s (±0.00%)    | 5.060s (±0.00%)    | +0.75%    | 34.382mb        | 34.382mb   | 0.00%       |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+

SimpleSetupBench
================

+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
|                                        | time (kde mode)                                     | memory                                     |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| subject                                | Tag: <current>     | Tag: base          | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| benchLoad1Event ()                     | 965.000μs (±0.00%) | 967.000μs (±0.00%) | -0.21%    | 34.198mb        | 34.198mb   | 0.00%       |
| benchLoad10000Events ()                | 54.612ms (±0.00%)  | 51.390ms (±0.00%)  | +6.27%    | 34.198mb        | 34.198mb   | 0.00%       |
| benchSave1Event ()                     | 1.011ms (±0.00%)   | 1.225ms (±0.00%)   | -17.46%   | 34.198mb        | 34.198mb   | 0.00%       |
| benchSave10000Events ()                | 218.499ms (±0.00%) | 221.429ms (±0.00%) | -1.32%    | 34.198mb        | 34.198mb   | 0.00%       |
| benchSave10000Aggregates ()            | 8.424s (±0.00%)    | 8.871s (±0.00%)    | -5.03%    | 34.198mb        | 34.198mb   | 0.00%       |
| benchSave10000AggregatesTransaction () | 4.866s (±0.00%)    | 4.848s (±0.00%)    | +0.38%    | 34.199mb        | 34.199mb   | 0.00%       |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+

SnapshotsBench
==============

+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
|                                        | time (kde mode)                                     | memory                                     |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| subject                                | Tag: <current>     | Tag: base          | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| benchLoad10000EventsMissingSnapshot () | 52.461ms (±0.00%)  | 53.102ms (±0.00%)  | -1.21%    | 34.273mb        | 34.273mb   | 0.00%       |
| benchLoad10000Events ()                | 911.100μs (±0.00%) | 912.400μs (±0.00%) | -0.14%    | 34.273mb        | 34.273mb   | 0.00%       |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+

SplitStreamBench
================

+-------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
|                         | time (kde mode)                                     | memory                                     |
+-------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| subject                 | Tag: <current>     | Tag: base          | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+-------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| benchLoad10000Events () | 4.409ms (±0.00%)   | 4.362ms (±0.00%)   | +1.08%    | 37.575mb        | 37.575mb   | 0.00%       |
| benchSave10000Events () | 351.039ms (±0.00%) | 345.366ms (±0.00%) | +1.64%    | 37.577mb        | 37.577mb   | 0.00%       |
+-------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+

SubscriptionEngineBatchBench
============================

+---------------------------+-------------------+-------------------+-----------+-----------------+------------+-------------+
|                           | time (kde mode)                                   | memory                                     |
+---------------------------+-------------------+-------------------+-----------+-----------------+------------+-------------+
| subject                   | Tag: <current>    | Tag: base         | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+---------------------------+-------------------+-------------------+-----------+-----------------+------------+-------------+
| benchHandle10000Events () | 76.078ms (±0.00%) | 77.433ms (±0.00%) | -1.75%    | 34.667mb        | 34.667mb   | 0.00%       |
+---------------------------+-------------------+-------------------+-----------+-----------------+------------+-------------+

SubscriptionEngineBench
=======================

+---------------------------+-----------------+-----------------+-----------+-----------------+------------+-------------+
|                           | time (kde mode)                               | memory                                     |
+---------------------------+-----------------+-----------------+-----------+-----------------+------------+-------------+
| subject                   | Tag: <current>  | Tag: base       | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+---------------------------+-----------------+-----------------+-----------+-----------------+------------+-------------+
| benchHandle10000Events () | 3.140s (±0.00%) | 3.153s (±0.00%) | -0.40%    | 46.815mb        | 46.815mb   | 0.00%       |
+---------------------------+-----------------+-----------------+-----------+-----------------+------------+-------------+

PersonalDataBench
=================

+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
|                                        | time (kde mode)                                     | memory                                     |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| subject                                | Tag: <current>     | Tag: base          | time-diff | Tag: <current>  | Tag: base  | memory-diff |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+
| benchLoad1Event ()                     | 889.800μs (±0.00%) | 891.900μs (±0.00%) | -0.24%    | 35.328mb        | 35.328mb   | 0.00%       |
| benchLoad10000Events ()                | 89.217ms (±0.00%)  | 94.195ms (±0.00%)  | -5.28%    | 35.328mb        | 35.328mb   | 0.00%       |
| benchSave1Event ()                     | 1.654ms (±0.00%)   | 1.494ms (±0.00%)   | +10.67%   | 35.328mb        | 35.328mb   | 0.00%       |
| benchSave10000Events ()                | 253.596ms (±0.00%) | 252.878ms (±0.00%) | +0.28%    | 35.331mb        | 35.331mb   | 0.00%       |
| benchSave10000Aggregates ()            | 13.247s (±0.00%)   | 13.448s (±0.00%)   | -1.49%    | 35.328mb        | 35.328mb   | 0.00%       |
| benchSave10000AggregatesTransaction () | 9.055s (±0.00%)    | 8.994s (±0.00%)    | +0.69%    | 35.829mb        | 35.829mb   | 0.00%       |
+----------------------------------------+--------------------+--------------------+-----------+-----------------+------------+-------------+

This comment gets update everytime a new commit comes in!

@DanielBadura DanielBadura linked an issue Feb 19, 2025 that may be closed by this pull request
Disallow Subscriber listening on the same event multiple times #655
Open
@DanielBadura DanielBadura marked this pull request as ready for review February 19, 2025 13:51
@DanielBadura DanielBadura changed the title Dont allow subcribing same event multiple times in one subscriber Dont allow subscribing same event multiple times in one subscriber Feb 19, 2025
DanielBadura added a commit that referenced this pull request Feb 19, 2025
@DanielBadura
Update API of SubscriberMetadata to not hold a list of methods for gi…
cec411e 
…ven events.

Ref: #701
@DanielBadura DanielBadura mentioned this pull request Feb 19, 2025
Draft
@DanielBadura DanielBadura merged commit d276fb1 into 3.10.x Feb 21, 2025
40 checks passed
@DanielBadura DanielBadura deleted the dont-allow-subcribe-double-event branch February 21, 2025 11:20
@DanielBadura DanielBadura added this to the 3.10.1 milestone Feb 21, 2025
@github-actions github-actions bot mentioned this pull request Feb 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidBadura DavidBadura DavidBadura approved these changes

Assignees

@DanielBadura DanielBadura

Labels
BC-Break bug Something isn't working
Projects
None yet
Milestone
3.10.1
Development

Successfully merging this pull request may close these issues.

Disallow Subscriber listening on the same event multiple times
2 participants
@DanielBadura @DavidBadura