Prevent PeerSet service to be buffered #1718

oxarbitrage · 2021-02-09T20:39:08Z

Motivation

Multiple calls to poll_ready()to the PeerSet service can cause fill ups and hangs if they are buffered.

Solution

The solution of this PR is to get the name of the service type as a string and check if that contains the word "Buffer" at runtime.
This uses https://doc.rust-lang.org/std/any/fn.type_name.html and by the notes it can fail at some edge cases.

Current and accepted type for the service is:

&mut tower::load::peak_ewma::PeakEwma<zebra_network::peer::client::Client

I manually tested this by changing the string to search from Buffer to PeakEwma, then started zebra and got the panic:

...
The application panicked (crashed).
Message:  assertion failed: !format!("{:?}", type_of(& service)).contains("PeakEwma")
...

The code in this pull request has:

Manually tested
Unit Tests

Review

@teor2345 when they get the time.

Related Issues

If the solution is good enough to be merged this will close #1697

Follow Up Work

Maybe do a test case in a separated issue.

oxarbitrage · 2021-02-09T20:39:40Z

zebra-network/src/peer_set/set.rs

+
+// Get the type of a variable as a string
+use std::any::type_name;
+fn type_of<T>(_: T) -> &'static str {


We will probably want to put this somewhere else if we end up keeping it.

Suggested change

fn type_of<T>(_: T) -> &'static str {

// replace with type_name_of_val when it stabilises

// https://github.com/rust-lang/rust/issues/66359

fn type_of<T>(_: T) -> &'static str {

@yaahc might have an idea where we want to move this, but it's ok for now.

Actually I think we can delete it and just check Self, see: https://github.com/ZcashFoundation/zebra/pull/1718/files#r573338707

teor2345

Looks good just a few minor tweaks

teor2345 · 2021-02-09T22:47:37Z

zebra-network/src/peer_set/set.rs

+
+                // `PeerSet` calls `poll_ready` multiple times on its `Client` services.
+                // `Buffer`ed polls can fill up their Buffer reservations, causing hangs.
+                // The next `assert` will make sure the service type is never `Buffer`. #1593


We can move part of the explanation into an assert message:

Suggested change

// The next `assert` will make sure the service type is never `Buffer`. #1593

// See #1593 for details.

teor2345 · 2021-02-09T22:50:16Z

zebra-network/src/peer_set/set.rs

+                // `PeerSet` calls `poll_ready` multiple times on its `Client` services.
+                // `Buffer`ed polls can fill up their Buffer reservations, causing hangs.
+                // The next `assert` will make sure the service type is never `Buffer`. #1593
+                assert!(!format!("{:?}", type_of(&service)).contains("Buffer"));


Suggested change

assert!(!format!("{:?}", type_of(&service)).contains("Buffer"));

assert!(!format!("{:?}", type_of(&service)).contains("Buffer"),

"Clients must not use tower::Buffer, because PeerSet uses poll_ready multiple times before each call, which can cause hangs with buffers" );

teor2345 · 2021-02-09T22:50:45Z

zebra-network/src/peer_set/set.rs

@@ -423,6 +423,12 @@ where
                    .ready_services
                    .get_index_mut(index)
                    .expect("preselected index must be valid");
+
+                // `PeerSet` calls `poll_ready` multiple times on its `Client` services.


Now in the assert message:

Suggested change

// `PeerSet` calls `poll_ready` multiple times on its `Client` services.

teor2345 · 2021-02-09T22:51:18Z

zebra-network/src/peer_set/set.rs

+
+// Get the type of a variable as a string
+use std::any::type_name;
+fn type_of<T>(_: T) -> &'static str {


@yaahc might have an idea where we want to move this, but it's ok for now.

Co-authored-by: teor <teor@riseup.net>

teor2345 · 2021-02-09T23:57:43Z

zebra-network/src/peer_set/set.rs

+
+                // `Buffer`ed polls can fill up their Buffer reservations, causing hangs.
+                // See #1593 for details.
+                assert!(!format!("{:?}", type_of(&service)).contains("Buffer"),
+                    "Clients must not use tower::Buffer, because PeerSet uses 
+                     poll_ready multiple times before each call, which can cause hangs with buffers");


The peer set type is static, so we only need to check it once.

Let's move it to the start of PeerSet::new, and do the checks on Self, so we don't have to use type_of:

Suggested change

// `Buffer`ed polls can fill up their Buffer reservations, causing hangs.

// See #1593 for details.

assert!(!format!("{:?}", type_of(&service)).contains("Buffer"),

"Clients must not use tower::Buffer, because PeerSet uses

poll_ready multiple times before each call, which can cause hangs with buffers");

// Using `poll_ready` without `call` fills up `Buffer` reservations, causing hangs.

// See #1593 for details.

assert!(!type_name::<Self>().contains("Buffer"),

"Clients must not use tower::Buffer, because PeerSet uses

`poll_ready` multiple times before each `call`, which causes buffer hangs");

Self (title case) is the current type:
https://doc.rust-lang.org/std/keyword.Self.html

self (lower case) is the current object:
https://doc.rust-lang.org/std/keyword.self.html

I dont think this will work. I added dbg!(type_name::<Self>()); as the first line in new() and here is the type i am getting (it contains the "Buffer" string):

[zebra-network/src/peer_set/set.rs:126] type_name::<Self>() = "zebra_network::peer_set::set::PeerSet<tower::load::peak_ewma::PeakEwmaDiscover<futures_util::stream::stream::filter::Filter<futures_channel::mpsc::Receiver<core::result::Result<tower::discover::Change<std::net::addr::SocketAddr, zebra_network::peer::client::Client>, alloc::boxed::Box<dyn std::error::Error+core::marker::Sync+core::marker::Send>>>, futures_util::future::ready::Ready<bool>, zebra_network::peer_set::initialize::init<tower::load_shed::LoadShed<tower::buffer::service::Buffer<zebrad::components::inbound::Inbound, zebra_network::protocol::internal::request::Request>>>::{{closure}}::{{closure}}>>>"

You're right, we don't need to check the outer PeerSet type, we need to check the inner D::Service type.

The previous code did by check using the service returned by:

let (key, service) = self .ready_services .get_index_mut(index) .expect("preselected index must be valid");

So we should be able to do:

Suggested change

// `Buffer`ed polls can fill up their Buffer reservations, causing hangs.

// See #1593 for details.

assert!(!format!("{:?}", type_of(&service)).contains("Buffer"),

"Clients must not use tower::Buffer, because PeerSet uses

poll_ready multiple times before each call, which can cause hangs with buffers");

// Using `poll_ready` without `call` fills up `Buffer` reservations, causing hangs.

// See #1593 for details.

assert!(!type_name::<D::Service>().contains("Buffer"),

"Clients must not use tower::Buffer, because PeerSet uses `poll_ready` multiple times before each `call`, which causes buffer hangs");

The type of ready_services is...

zebra/zebra-network/src/peer_set/set.rs

Line 89 in 1569824

ready_services: IndexMap<D::Key, D::Service>,

yaahc

I don't really love the idea of using type_name comparisons for this assertion, it seems like an indirect way to enforce the invariant we care about, and I'm worried about it breaking because the language makes no guarantees about the stability of type names.

The returned string must not be considered to be a unique identifier of a type as multiple types may map to the same type name. Similarly, there is no guarantee that all parts of a type will appear in the returned string: for example, lifetime specifiers are currently not included. In addition, the output may change between versions of the compiler.

I am still thinking of alternatives but to start I think one option that may be better is to use Clone as a proxy for Buffer, since Buffer/Batch are the only types that should introduce "clone-ness" to Services. We'd then use static_assertions to assert that D::Service does not implement Clone. Now, I'm not super happy with this solution either, since it is similarly indirect, but I'm more confident in its reliability, and it doesn't introduce any runtime checks. edit: this won't work due to parametricity

yaahc · 2021-02-10T01:54:23Z

Lets hold off on merging this, I'm still figuring out the details but I'm talking to @hawkw right now and I think we're going to fix the issue directly so its no longer possible to misuse Buffer in a way that exhausts all the reservations, then this assertion will be unnecessary.

oxarbitrage · 2021-02-10T01:56:46Z

Sounds good

teor2345 · 2021-02-10T04:43:13Z

I've marked this PR as draft pending the tower::Buffer fix.

mpguerra · 2021-02-11T10:35:16Z

I've marked this PR as draft pending the tower::Buffer fix.

Is the fix you are referring to the one for issue #1671 ?

teor2345 · 2021-02-11T21:31:56Z

I've marked this PR as draft pending the tower::Buffer fix.

Is the fix you are referring to the one for issue #1671 ?

No. #1671 fixes tower-batch, a zebra crate which is based on tower::Buffer, and shares some of its issues. The PeerSet service does not use tower-batch.

The fixes I am referring to were made in the tower crate a while ago (tower-rs/tower#476 and tower-rs/tower#480). But we didn't know these bugs were fixed until yesterday, because the tower::Buffer comments were outdated, and the tower::Buffer tests were incomplete.

Since tower is already fixed, this PR and its ticket are obsolete.

teor2345 · 2021-02-15T01:16:23Z

This PR belongs in sprint 3 because we actually did the work.

assert when peerset is buffered

8b92ddd

zfnd-bot bot assigned oxarbitrage Feb 9, 2021

oxarbitrage commented Feb 9, 2021

View reviewed changes

teor2345 suggested changes Feb 9, 2021

View reviewed changes

add suggestions from code review

626a315

Co-authored-by: teor <teor@riseup.net>

teor2345 reviewed Feb 9, 2021

View reviewed changes

yaahc suggested changes Feb 10, 2021

View reviewed changes

oxarbitrage added 3 commits February 9, 2021 22:34

use type_name() and remove type_of()

e356264

move the assert outside the loop

82bd8a7

remove newline

3e2f7b5

teor2345 marked this pull request as draft February 10, 2021 04:38

teor2345 added the S-needs-triage Status: A bug report needs triage label Feb 10, 2021

teor2345 closed this Feb 11, 2021

teor2345 added this to the 2021 Sprint 3 milestone Feb 15, 2021

mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent PeerSet service to be buffered #1718

Prevent PeerSet service to be buffered #1718

oxarbitrage commented Feb 9, 2021

oxarbitrage Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 left a comment

teor2345 Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 Feb 9, 2021

teor2345 Feb 10, 2021

oxarbitrage Feb 10, 2021

teor2345 Feb 10, 2021

teor2345 Feb 10, 2021 •

edited

Loading

yaahc left a comment •

edited

Loading

yaahc commented Feb 10, 2021

oxarbitrage commented Feb 10, 2021

teor2345 commented Feb 10, 2021

mpguerra commented Feb 11, 2021

teor2345 commented Feb 11, 2021

teor2345 commented Feb 15, 2021

	// The next `assert` will make sure the service type is never `Buffer`. #1593
	// See #1593 for details.

	assert!(!format!("{:?}", type_of(&service)).contains("Buffer"));
	assert!(!format!("{:?}", type_of(&service)).contains("Buffer"),
	"Clients must not use tower::Buffer, because PeerSet uses poll_ready multiple times before each call, which can cause hangs with buffers" );

Prevent PeerSet service to be buffered #1718

Prevent PeerSet service to be buffered #1718

Conversation

oxarbitrage commented Feb 9, 2021

Motivation

Solution

Review

Related Issues

Follow Up Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

yaahc left a comment • edited Loading

Choose a reason for hiding this comment

yaahc commented Feb 10, 2021

oxarbitrage commented Feb 10, 2021

teor2345 commented Feb 10, 2021

mpguerra commented Feb 11, 2021

teor2345 commented Feb 11, 2021

teor2345 commented Feb 15, 2021

teor2345 Feb 10, 2021 •

edited

Loading

yaahc left a comment •

edited

Loading