Skip to content

rpc server: fix subscription id_provider being reset to default one.#6588

Merged
niklasad1 merged 9 commits intoparitytech:masterfrom
TarekkMA:patch-1
Nov 25, 2024
Merged

rpc server: fix subscription id_provider being reset to default one.#6588
niklasad1 merged 9 commits intoparitytech:masterfrom
TarekkMA:patch-1

Conversation

@TarekkMA
Copy link
Copy Markdown
Contributor

@TarekkMA TarekkMA commented Nov 21, 2024

Description

The PR ensures that the id_provider variable is cloned instead of taken, which can help prevent issues related id provider being reset to the default.

In a test in moonbeam we found that the id_provider is being reset somehow and changed to the default one. Changing .take() to .clone() would fix the issue.

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@TarekkMA TarekkMA marked this pull request as ready for review November 25, 2024 12:51
Copy link
Copy Markdown
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good to me, good catch.

For other reviewers this id_provider.take() was used in the connection loop and caused the first connection use the proper id_provider but after that the all other connections would fallback to the default one because the Option was None after that point.

My bad

@niklasad1 niklasad1 changed the title fix: id_provider being reset to default one. rpc server: fix subsription id_provider being reset to default one. Nov 25, 2024
@niklasad1 niklasad1 added the T0-node This PR/Issue is related to the topic “node”. label Nov 25, 2024
@niklasad1 niklasad1 changed the title rpc server: fix subsription id_provider being reset to default one. rpc server: fix subscription id_provider being reset to default one. Nov 25, 2024
@niklasad1 niklasad1 added the A3-backport Pull request is already reviewed well in another branch. label Nov 25, 2024
@niklasad1
Copy link
Copy Markdown
Contributor

@TarekkMA just remove mut here then it should good to go

@niklasad1 niklasad1 enabled auto-merge November 25, 2024 16:28
@niklasad1 niklasad1 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into paritytech:master with commit 6d5f814 Nov 25, 2024
niklasad1 added a commit that referenced this pull request Nov 26, 2024
…6588)

# Description

The PR ensures that the id_provider variable is cloned instead of taken,
which can help prevent issues related id provider being reset to the
default.

In [a test in
moonbeam](https://github.com/moonbeam-foundation/moonbeam/blob/c6d07d703dfcdd94cc311fa83b553071b7d433ff/test/suites/dev/moonbase/test-subscription/test-subscription.ts#L20-L31)
we found that the id_provider is being reset somehow and changed to the
default one. Changing .take() to .clone() would fix the issue.


# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [ ] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [ ] I have made corresponding changes to the documentation (if
applicable)
* [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
niklasad1 added a commit that referenced this pull request Nov 26, 2024
…6588)

# Description

The PR ensures that the id_provider variable is cloned instead of taken,
which can help prevent issues related id provider being reset to the
default.

In [a test in
moonbeam](https://github.com/moonbeam-foundation/moonbeam/blob/c6d07d703dfcdd94cc311fa83b553071b7d433ff/test/suites/dev/moonbase/test-subscription/test-subscription.ts#L20-L31)
we found that the id_provider is being reset somehow and changed to the
default one. Changing .take() to .clone() would fix the issue.


# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [ ] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [ ] I have made corresponding changes to the documentation (if
applicable)
* [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
niklasad1 added a commit that referenced this pull request Nov 26, 2024
…6588)

# Description

The PR ensures that the id_provider variable is cloned instead of taken,
which can help prevent issues related id provider being reset to the
default.

In [a test in
moonbeam](https://github.com/moonbeam-foundation/moonbeam/blob/c6d07d703dfcdd94cc311fa83b553071b7d433ff/test/suites/dev/moonbase/test-subscription/test-subscription.ts#L20-L31)
we found that the id_provider is being reset somehow and changed to the
default one. Changing .take() to .clone() would fix the issue.


# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [ ] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [ ] I have made corresponding changes to the documentation (if
applicable)
* [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Krayt78 pushed a commit to Krayt78/polkadot-sdk that referenced this pull request Dec 18, 2024
…aritytech#6588)

# Description

The PR ensures that the id_provider variable is cloned instead of taken,
which can help prevent issues related id provider being reset to the
default.

In [a test in
moonbeam](https://github.com/moonbeam-foundation/moonbeam/blob/c6d07d703dfcdd94cc311fa83b553071b7d433ff/test/suites/dev/moonbase/test-subscription/test-subscription.ts#L20-L31)
we found that the id_provider is being reset somehow and changed to the
default one. Changing .take() to .clone() would fix the issue.


# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [ ] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [ ] I have made corresponding changes to the documentation (if
applicable)
* [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
…aritytech#6588)

# Description

The PR ensures that the id_provider variable is cloned instead of taken,
which can help prevent issues related id provider being reset to the
default.

In [a test in
moonbeam](https://github.com/moonbeam-foundation/moonbeam/blob/c6d07d703dfcdd94cc311fa83b553071b7d433ff/test/suites/dev/moonbase/test-subscription/test-subscription.ts#L20-L31)
we found that the id_provider is being reset somehow and changed to the
default one. Changing .take() to .clone() would fix the issue.


# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [ ] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [ ] I have made corresponding changes to the documentation (if
applicable)
* [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A3-backport Pull request is already reviewed well in another branch. T0-node This PR/Issue is related to the topic “node”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants