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

[3006.x] Do not allow duplicate message ids #65247

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

dwoz
Copy link
Contributor

@dwoz dwoz commented Sep 20, 2023

What does this PR do?

What issues does this PR fix or reference?

Fixes: #65114

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@dwoz dwoz requested a review from a team as a code owner September 20, 2023 21:34
@dwoz dwoz requested review from twangboy and removed request for a team September 20, 2023 21:34
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title [wip] Do not allow duplicate message ids [3006.x][wip] Do not allow duplicate message ids Sep 20, 2023
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:13 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:13 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:13 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:13 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:58 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:58 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:58 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 22:58 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 23:17 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 20, 2023 23:18 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 01:40 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 01:40 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 01:40 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 01:40 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 01:41 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 01:41 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 02:04 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 02:04 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 02:04 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 02:04 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci September 21, 2023 02:04 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 02:31 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 02:31 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 04:45 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 04:45 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 04:45 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 04:45 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 04:45 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 04:45 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 15:34 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 15:34 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 15:34 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 15:34 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 15:34 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 26, 2023 15:34 — with GitHub Actions Inactive
super().__init__(**kwargs)
self.opts = opts
self.socket_queue = socket_queue
self._socket = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self._socket = None
self._socket = None
self.register_finalize_method(self.close)

Comment on lines +156 to +158
# pylint: disable=W1701
def __del__(self):
self.close()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# pylint: disable=W1701
def __del__(self):
self.close()

With the register_finalize_method change above, this is no longer necessary.

@dwoz dwoz temporarily deployed to ci October 29, 2023 06:09 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 29, 2023 06:09 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 29, 2023 06:09 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 29, 2023 06:09 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 29, 2023 06:09 — with GitHub Actions Inactive
@dwoz dwoz temporarily deployed to ci October 29, 2023 06:09 — with GitHub Actions Inactive
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My review comments will be addressed in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test:full Run the full test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants