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

Plumb RPC listener up to caller #5038

Merged

Conversation

nickvikeras
Copy link
Contributor

@nickvikeras nickvikeras commented Jul 16, 2024

Description

This PR allows the RPC server's socket address to be returned when initializing the server. This allows the library consumer to easily programmatically determine which port the RPC server is listening on. My use case for this is automated testing. I'd like to be able to simply specify that the server bind to port '0' and then test against whatever port the OS assigns dynamically. I will have many RPC servers running in parallel across many tests within a single process, and I don't want to have to deal with port conflicts.

Integration

Integration is straightforward. My main concern is that I am making non-backwards-compatible changes to public library functions. Let me know if I should leave backwards-compatible wrappers in place for any/all of the public functions that were modified.

Review Notes

The rationale for making the new listen_addresses field on the RpcHandlers struct a [MultiAddr] rather than SocketAddr is because I wanted it to be transport-agnostic as well as capable of supporting multiple listening addresses in case that is ever required by the RPC server in the future.

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)
  1. I didn't understand what the 'T' label meant. Am I supposed to open a github Issue for my PR?
  2. I didn't see an easy way to add tests since the functions I am modifying are not directly called by any tests.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 16, 2024

User @nickvikeras, please sign the CLA here.

@bkchr bkchr requested a review from niklasad1 July 16, 2024 20:51
@nickvikeras nickvikeras requested a review from gui1117 July 18, 2024 15:55
None => vec![],
};

let rpc_handlers = RpcHandlers {
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't makes sense to add listen_addresses to RpcHandlers this just an in-memory way to perform rpc calls, please remove it RpcHandlers.

Why did you add it (maybe I'm missing something)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did you add it (maybe I'm missing something)?

Just because the RpcHandlers type is what is getting returned to the caller. Would a new return type that can provide both the Server and the RpcHandlers be preferred?

Copy link
Member

Choose a reason for hiding this comment

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

Aight, I see one may want to know the listen addrs on the rpc endpoint after calling spawn_tasks.

It's probably fine to keep the RpcHandlers or rename to the RpcService or something, but it needs to documented that is represent a running RPC server as well with the legacy in-memory rpc calls.

We may want to remove latter at some point, it was for WASM stuff before smoldot was a thing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some notes with a link to this comment thread.

@@ -372,7 +382,7 @@ pub fn start_rpc_servers<R>(
config: &Configuration,
gen_rpc_module: R,
rpc_id_provider: Option<Box<dyn RpcSubscriptionIdProvider>>,
) -> Result<Box<dyn std::any::Any + Send + Sync>, error::Error>
) -> Result<(Box<dyn std::any::Any + Send + Sync>, Option<SocketAddr>), error::Error>
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this.

But the return type is bit complicated and I don't remember why it's Box<dyn Any> but would be much nicer with a wrapper type that has an API to get the listen_addr instead returning a tuple.... because the Server type already has this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used the Server type here, and moved the Drop wrapper up to the point where it was actually needed. Let me know if that suffices.

@niklasad1 niklasad1 requested review from a team and removed request for gui1117 August 6, 2024 04:51
Copy link
Member

@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.

nice work, looks good to me

@niklasad1 niklasad1 added the T0-node This PR/Issue is related to the topic “node”. label Aug 6, 2024
@github-actions github-actions bot requested a review from niklasad1 August 6, 2024 12:35
Copy link

github-actions bot commented Aug 6, 2024

Review required! Latest push from author must always be reviewed

@niklasad1
Copy link
Member

hey @nickvikeras

This PR needs a prdoc description, instructions how write and generate such here the label should be node dev. Can you provide such a thing?

I can help with that if you don't get it working.

@github-actions github-actions bot requested a review from niklasad1 August 7, 2024 18:04
@nickvikeras
Copy link
Contributor Author

hey @nickvikeras

This PR needs a prdoc description, instructions how write and generate such here the label should be node dev. Can you provide such a thing?

I can help with that if you don't get it working.

I added it in the latest commit. Let me know if it looks good.

@nickvikeras
Copy link
Contributor Author

nickvikeras commented Aug 23, 2024

Gentle ping. Looks like this needs one more approval before it can be merged.

@niklasad1
Copy link
Member

Hey @nickvikeras

Sorry, I merged #4792 and causes some merge conflicts that you have fix.
Let me know if you need some help with that.

@nickvikeras
Copy link
Contributor Author

@ niklasad1 sorry for the delay here. I just resolved the merge conflicts. I think this is good to merge again.

@michalkucharczyk michalkucharczyk requested a review from a team September 6, 2024 12:00
Copy link
Member

@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.

Hey again,

Just to fix the waiting stuff then it should be good to go, sorry missed that

@niklasad1 niklasad1 added this pull request to the merge queue Sep 9, 2024
Merged via the queue into paritytech:master with commit c72f9ab Sep 9, 2024
201 of 203 checks passed
@nickvikeras
Copy link
Contributor Author

Thank you!!! 🎉

@nickvikeras nickvikeras deleted the vikeras-rpc-listener-plumbing branch September 9, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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