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

HTTP server with tower interaction #831

Merged
merged 37 commits into from
Aug 12, 2022
Merged

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jul 25, 2022

This PR allows users to set custom tower ServiceBuilders as middleware
to layer the RPC service.

Exposing such functionality enables users to rely on crates that build upon tower,
such as tower_http to crate a default layered filtering system (ie, enable CORS from
tower_http, or extensive debugging upon receiving a response / request).

Tower HTTP Example

The server is constructed with the tower's ServiceBuilder with the following layer on top:

.layer(
	TraceLayer::new_for_http()
		.on_body_chunk(|chunk: &Bytes, latency: Duration, _: &tracing::Span| {
			tracing::trace!(size_bytes = chunk.len(), latency = ?latency, "sending body chunk")
		})
		.make_span_with(DefaultMakeSpan::new().include_headers(true))
		.on_response(DefaultOnResponse::new().include_headers(true).latency_unit(LatencyUnit::Micros)),
)

Output with RUST_LOG=trace:

2022-07-27T12:48:46.665907Z TRACE request{method=POST
uri=/ version=HTTP/1.1 headers={"content-type": "application/json",
"accept": "application/json", "host": "127.0.0.1:9935", "content-length": "45"}}:
tower_http: sending body chunk size_bytes=38 latency=347.166µs

Next Steps

  • Documentation
  • Enable service with tower_http

Closes #841

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner July 25, 2022 15:02
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
http-server/src/server.rs Outdated Show resolved Hide resolved
http-server/src/server.rs Outdated Show resolved Hide resolved
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
http-server/src/server.rs Outdated Show resolved Hide resolved
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…r_v4

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv changed the title [WIP] HTTP server with tower interaction HTTP server with tower interaction Aug 8, 2022
@lexnv lexnv self-assigned this Aug 8, 2022
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
http-server/src/server.rs Outdated Show resolved Hide resolved
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.

great, we should probably check how much overhead tower introduces.

follow-up PR remove the CORS stuff and rely on tower?!

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
http-server/src/server.rs Outdated Show resolved Hide resolved
/// This is similar to [`hyper::service::service_fn`].
#[derive(Debug)]
pub struct TowerService<L> {
inner: ServiceData<L>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a benefit to having this inner ServiceData<L> as well as the outer TowerService<L> instead of just having TowerService<L> have all of the props etc from ServiceData in it?

Copy link
Member

Choose a reason for hiding this comment

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

orphan rule?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really follow, but having the inner type isn't a big deal anyway in any case!

Copy link
Member

Choose a reason for hiding this comment

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

Look below it implements hyper::service::Service<hyper::Request<hyper::Body>> which can't be done because both the trait and type are not defined by "us".

So one need the newtype to implement that trait AFAIU.

}

fn call(&mut self, request: hyper::Request<hyper::Body>) -> Self::Future {
let data = self.inner.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have to worry about performance at all cloning all of these?

(It looked like a bunch of them were cloned previously so I'm guessing not).

And ah ok, is the inner struct so that you can clone it here? I guess you can just clone &mut self though and have just one struct?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's similar as it was before because of the FnMut closure in the hyper service fn

http-server/src/server.rs Outdated Show resolved Hide resolved
niklasad1 and others added 2 commits August 12, 2022 12:29
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM, nice work Alex!

My remaining comments are nits and can be ignored!

@niklasad1 niklasad1 merged commit 80bcef0 into master Aug 12, 2022
@niklasad1 niklasad1 deleted the poc_middleware_layer_v4 branch August 12, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support tower_http Service's in front of our Http server
3 participants