replace WS and HTTP servers with a server that supports both WS and HTTP
#863
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
niklasad1
changed the title
ws server support both WS and HTTP WS and HTTP servers with a server that supports both WS and HTTP
niklasad1
force-pushed
the
na-mixed-ws-http-server-poc
branch
2 times, most recently
from
2d4b34b
to
4bbb81c
Compare
|
benches |
jsdw
reviewed
Aug 31, 2022
jsdw
reviewed
Aug 31, 2022
server/src/transport/ws.rs
Outdated
| /// | ||
| /// Returns `(MethodResponse, None)` on every call that isn't a subscription | ||
| /// Otherwise `(MethodResponse, Some(PendingSubscriptionCallTx)`. | ||
| pub(crate) async fn execute_call<L: WsLogger>(c: Call<'_, L>) -> MethodResult { |
There was a problem hiding this comment.
How much similarity is there between these methods and the same in http.rs; is it worth trying to reuse more functionality from within them or are the yactually fairly distinct?
There was a problem hiding this comment.
yeah those are super similar but WS stuff has some extra stuff such as subscriptions but I think we could rework that in a follow-up PR.
There was a problem hiding this comment.
Makes sense to me; definitely no rush, just something I pondered when I saw it!
niklasad1
force-pushed
the
na-mixed-ws-http-server-poc
branch
from
47fa4b2
to
bc054f8
Compare
CORS has been removed to tower middleware and doesn't need to supported anymore
niklasad1
changed the title
WS and HTTP servers with a server that supports both WS and HTTPWS and HTTP servers with a server that supports both WS and HTTP
jsdw
reviewed
Sep 1, 2022
| @@ -0,0 +1,6 @@ | |||
| //! Host filtering. | |||
There was a problem hiding this comment.
Is the idea that in a followup PR we might be able to move this stuff into a tower middleware?
There was a problem hiding this comment.
yeah, if it will become cleaner
it's so little code now so I'm okay keeping it here as well.
jsdw
reviewed
Sep 12, 2022
jsdw
reviewed
Sep 12, 2022
Comment on lines
+57
to
+75
| // WebSocket. | ||
| { | ||
| let client = WsClientBuilder::default().build(format!("ws://{}", addr)).await?; | ||
| let response: String = client.request("say_hello", rpc_params![]).await?; | ||
| println!("[main]: ws response: {:?}", response); | ||
| let _response: Result<String, _> = client.request("unknown_method", rpc_params![]).await; | ||
| let _ = client.request::<String, _>("say_hello", rpc_params![]).await?; | ||
| } | ||
|
|
||
| let client = HttpClientBuilder::default().build(&url)?; | ||
| let response: String = client.request("say_hello", rpc_params![]).await?; | ||
| println!("[main]: response: {:?}", response); | ||
| let _response: Result<String, _> = client.request("unknown_method", rpc_params![]).await; | ||
| let _: String = client.request("say_hello", rpc_params![]).await?; | ||
| tokio::time::sleep(std::time::Duration::from_secs(1)).await; | ||
|
|
||
| // HTTP. | ||
| { | ||
| let client = HttpClientBuilder::default().build(format!("http://{}", addr))?; | ||
| let response: String = client.request("say_hello", rpc_params![]).await?; | ||
| println!("[main]: http response: {:?}", response); | ||
| let _response: Result<String, _> = client.request("unknown_method", rpc_params![]).await; | ||
| let _ = client.request::<String, _>("say_hello", rpc_params![]).await?; | ||
| } |
There was a problem hiding this comment.
Nit: I wonder whether it's worth having a separate http_and_ws.rs example to show that you can do this, since this example is, I guess, focused on middleware!
Doesn't need doing in this PR though; just a thought :)
There was a problem hiding this comment.
Oh actually I see what you're getting at here; middleware works for either http or ws. Let's leave it be :)
jsdw
reviewed
Sep 12, 2022
jsdw
reviewed
Sep 12, 2022
server/src/server.rs
Outdated
| // NOTE: this task will finish after the HTTP request have been finished. | ||
| // | ||
| // Thus, if it was a Websocket Upgrade request the background task will be spawned separately. | ||
| let mut stop_handle2 = stop_handle.clone(); |
There was a problem hiding this comment.
Nit: does it need cloning? it's not being used anywhere elseby the looks of it :)
Is the comment saying that the async block below finishes when the http request is read and handled but that ws will lead to task being spawned?
There was a problem hiding this comment.
it's in a loop so the clone is needed :(
it was just a side-note to previous behavior i.e, the task would live as long as the ws connection task was alive but now "this task" finished once the HTTP request is finished but I removed the comment.
this is because the hyper callback just a spawns the task directly without access to FutureDriver
|
Looks great! Just to document; me and Niklas chatted about having a method on Asdie from that I'm good to tick this PR; it looks awesome! |
niklasad1
commented
Sep 13, 2022
| fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| let mut shutdown_waiter = ShutdownWaiter(self.0.clone()); | ||
| /// Wait for the server to stopped.. | ||
| pub async fn stopped(self) { |
There was a problem hiding this comment.
we could call this wait or something not sure :)
There was a problem hiding this comment.
I like stopped offhand just because it is consistent with stopped and stop!
niklasad1
commented
Sep 13, 2022
| tokio::select! { | ||
| res = &mut conn => { | ||
| match res { | ||
| Ok(_) => tracing::info!("Accepting new connection {}/{}", curr_conns, max_conns), |
There was a problem hiding this comment.
shall we log the peer addr here as well? I reckon that it might be useful
There was a problem hiding this comment.
That sounds like a good idea, we can always come with another PR
Wondering if we should incorporate the peer addr or connection address to uniquely identify which logs are coming from which connections?
It might help us group errors and warnings that have a common root cause
There was a problem hiding this comment.
yeah, it was one thing missing from the logs when I tried to debug these Transport(i/o error: unexpected end of file) errors that we have seen plenty of
There was a problem hiding this comment.
let's do it in another PR
niklasad1
commented
Sep 13, 2022
jsdw
reviewed
Sep 13, 2022
lexnv
reviewed
Sep 13, 2022
lexnv
reviewed
Sep 13, 2022
lexnv
approved these changes
Sep 13, 2022
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
This was referenced Sep 14, 2022
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #821 and #869