fixed ipc leak by debris · Pull Request #277 · paritytech/jsonrpc

debris · 2018-06-06T12:09:10Z

closes #275
closes openethereum/parity-ethereum#8774
~~openethereum/parity-ethereum#8618~~

The leak existed, cause we were merging incoming message responses stream with outgoing messages. If either of them existed, we were not dropping a connection.

To fix the issue, we finish the merged stream as soon as one of the merged streams ends.

debris · 2018-06-06T12:09:30Z

thanks @gnunicorn for debugging the issue!

svyatonik · 2018-06-06T12:28:33Z

+
+		self.flag = !self.flag;
+
+		match b.poll()? {


Am I right that this whole match can be replaced with simple b.poll() call? :)

🤦‍♂️ 👍

gnunicorn · 2018-06-06T12:30:55Z

closes openethereum/parity-ethereum#8774
closes openethereum/parity-ethereum#8618

Just pointing out, that this doesn't come into effect until Paritiy's cargo.toml branch references are upgraded, too as they don't point to master (I suspect this ther is a branching whenever there is a release?).

debris · 2018-06-06T12:39:04Z

@gnunicorn I know, we need to backport these fixes to that branch

debris · 2018-06-06T12:52:47Z

hold on with merging this pr, tests are randomly failing. Most likely because server is now released before the end of the test

dvdplm

Looks good (and gnunicorns bug report was amazing!) but I get randomly failing tests in ipc: sometimes all is green, but most of the times there's at least one failure. Can you take a look at that?

dvdplm · 2018-06-06T13:13:30Z

 				})
 				.filter_map(|x| x)
-				.select(receiver.map_err(|e| {
+				.select_both(receiver.map_err(|e| {


I for one would appreciate a comment as to why we're using select_both here – doesn't seem super-obvious! ;)

debris · 2018-06-06T14:00:55Z

Io it seems like tests are failing, because every request uses it's own, new event loop. And order of the event loop initialization is not guaranteed.

So sometimes tests fail, because server hasn't started yet or started and was already dropped (because nothing has an ownership of the event loop)

c0deright · 2018-06-06T14:40:45Z

Hold your beer - it seems this doesn't fix openethereum/parity-ethereum#8618. Please see openethereum/parity-ethereum#8618 (comment)

debris · 2018-06-06T14:44:33Z

🍺

debris

ipc server tests are just terrible, contain multiple bugs and they should be rewritten

debris · 2018-06-06T15:20:25Z

 			"Response does not exactly match the expected response",
-			);
+		);
+		server.close();


test issue no. 1

we were dropping the server before we even connected to it. everything was working only because of the leak

debris · 2018-06-06T15:23:54Z

-		let _server = builder.start(path).expect("Server must run with no issues");
-		thread::sleep(::std::time::Duration::from_millis(50));
+		let server = builder.start(path).expect("Server must run with no issues");
+		thread::sleep(::std::time::Duration::from_millis(5000));


test issue no. 2 (not solved)

server is not started synchronously, therefore we cannot assume that it's already available at the time of making the request. fixing this issue is actually quite complex, so I added only this workaround

rphmeier · 2018-06-06T15:31:54Z

+	type Error = S1::Error;
+
+	fn poll(&mut self) -> Poll<Option<S1::Item>, S1::Error> {
+		let (a, b) = if self.flag {


eating a virtual dispatch on every call to poll doesn't seem great to me.

(a macro poll_inner!(a, b) would probably end up being less verbose in the end)

it is just modified the version of select from futures library https://docs.rs/futures/0.1.21/src/futures/stream/select.rs.html#11-15

rphmeier · 2018-06-06T15:32:38Z

+			Async::NotReady => (),
+		};
+
+		self.flag = !self.flag;


is this second negation of self.flag intended?

dvdplm · 2018-06-06T19:06:26Z

@@ -332,13 +339,15 @@ mod tests {



This test still occasionally fails here. Running for i in {1..20}; do cargo test; done it fails 2-3 times. Running it on by itself (for i in {1..20}; do cargo test req_parallel; done) makes things worse: about a fourth fails.

dvdplm

~~LGTM. Minor grumbles.~~

EDIT I can still get req_parallel to fail on Rust stable (nightly seem fine). :(

dvdplm · 2018-06-11T12:38:06Z

 		let server = run(path);
-		thread::sleep(::std::time::Duration::from_millis(1000));
+		thread::sleep(Duration::from_millis(100));
+		let (stop_signal, stop_receiver) = mpsc::channel(400);


How about we parametrize the nr of threads/loops so we can do mpsc::channel(TEST_THRDS * TEST_ITERS);?

dvdplm · 2018-06-11T12:38:54Z

 				})
 			);
 		}
+		thread::sleep(Duration::from_millis(100));


Just curious: why do we still need to nap?

Tests still failing on Rust stable

debris · 2018-06-12T06:09:34Z

this issue is super annoying. it works on my machine always now. But it looks like the server is not yet ready, when the tests start

dvdplm · 2018-06-12T09:10:00Z

it works on my machine always now

I still get failures, but only using 1.26.1. No failures on nightly.

debris · 2018-06-12T12:18:12Z


 [target.'cfg(not(windows))'.dev-dependencies]
-tokio-uds = "0.1"
+tokio-uds = "0.2"


Updating the package helped me diagnose tests issues

debris · 2018-06-12T12:19:11Z

-				.select(receiver.map_err(|e| {
+				// we use `select_with_weak` here, instead of `select`, to close the stream
+				// as soon as the ipc pipe is closed
+				.select_with_weak(receiver.map_err(|e| {


maintains the stream as long as the connection exists. the outgoing stream may be closed earlier

debris · 2018-06-12T12:20:08Z


 				Ok(())
 			});
+			start_signal.send(Ok(())).expect("Cannot fail since receiver never dropped before receiving");


start_signal was triggered before the server was fully set up. After fixing the order of initialisation, we no longer need thread::sleep in tests

* fixed ipc connection leak, closes #275 * fixed indentation * fixed broken pipe issue in tests * empirical tests fixes * fix tests * fix tests * fix tests * move ipc start_signal.send after the incoming.for_each * log ipc traces on travis * keep writer in memory as long as possible * select_with_weak * remove redundant thread::sleep * test session end * fixed race condition in test_session_end

debris added 2 commits June 6, 2018 14:03

fixed ipc connection leak, closes #275

adc2f69

fixed indentation

a3e4401

svyatonik reviewed Jun 6, 2018

View reviewed changes

dvdplm suggested changes Jun 6, 2018

View reviewed changes

fixed broken pipe issue in tests

2ccdeb6

debris force-pushed the fixed_ipc_leak branch from 71db271 to 2ccdeb6 Compare June 6, 2018 15:17

debris commented Jun 6, 2018

View reviewed changes

rphmeier reviewed Jun 6, 2018

View reviewed changes

empirical tests fixes

feecb98

dvdplm reviewed Jun 6, 2018

View reviewed changes

fix tests

dd70e1f

dvdplm previously approved these changes Jun 11, 2018

View reviewed changes

fix tests

e39aca6

debris added 4 commits June 12, 2018 08:11

fix tests

93be6f1

move ipc start_signal.send after the incoming.for_each

5ce80c7

log ipc traces on travis

cf248b9

keep writer in memory as long as possible

98a7dc2

debris added 2 commits June 12, 2018 14:04

select_with_weak

8151fd5

remove redundant thread::sleep

15e6c03

debris commented Jun 12, 2018

View reviewed changes

test session end

a7630db

tomusdrw approved these changes Jun 12, 2018

View reviewed changes

fixed race condition in test_session_end

bb76cf3

debris merged commit ab2c608 into master Jun 12, 2018

debris deleted the fixed_ipc_leak branch June 12, 2018 16:03

debris mentioned this pull request Jun 12, 2018

update jsonrpc libs, fixed ipc leak openethereum/parity-ethereum#8876

Merged

Conversation

debris commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debris commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnunicorn commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debris commented Jun 6, 2018

Uh oh!

debris commented Jun 6, 2018

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debris commented Jun 6, 2018

Uh oh!

c0deright commented Jun 6, 2018

Uh oh!

debris commented Jun 6, 2018

Uh oh!

debris left a comment

Choose a reason for hiding this comment

Uh oh!

debris Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debris Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debris commented Jun 12, 2018

Uh oh!

dvdplm commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

debris commented Jun 6, 2018 •

edited

Loading

gnunicorn commented Jun 6, 2018 •

edited

Loading

debris Jun 6, 2018 •

edited

Loading

debris Jun 6, 2018 •

edited

Loading

dvdplm left a comment •

edited

Loading