fixes graceful termination when `aleth --test` is invoked but require… by christianparpart · Pull Request #5146 · ethereum/aleth

christianparpart · 2018-07-27T13:07:14Z

Fixes graceful termination when aleth --test is invoked but required a SIGKILL to terminate.

This was because the UnixSocketServer requires at least one accepting
connection after SIGTERM/SIGINT has been received, which usually never
is the case.

This PR implements a trivial workaround by first waiting for up to N
msecs (N=1000 for now) for any input to arrive, and if not, it'll just
retry, hence, evaluating m_running, resulting in aleth --test to
properly terminate upon receival of SIGINT or SIGTERM.

gumb0 · 2018-07-27T13:22:29Z

+	FD_ZERO(&out);
+	FD_ZERO(&err);
+	FD_SET(fd, &in);
+	struct timeval tv;


In a more C++ way this could be timeval const tv = { timeoutMillis / 1000, timeoutMillis % 1000 }

I was a little conservative. Thx. done! :-)

gumb0 · 2018-07-27T13:23:20Z

 	return false;
 }

+static bool waitForReadable(int fd, unsigned timeoutMillis)


Please move it into unnamed namespace above.

gumb0

Also please add a comment describing why it's needed

codecov-io · 2018-07-27T13:47:29Z

Codecov Report

Merging #5146 into develop will decrease coverage by 0.22%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           develop   #5146      +/-   ##
==========================================
- Coverage    59.92%   59.7%   -0.23%     
==========================================
  Files          337     337              
  Lines        27306   27271      -35     
  Branches      3173    3175       +2     
==========================================
- Hits         16362   16281      -81     
- Misses        9860    9906      +46     
  Partials      1084    1084

gumb0 · 2018-07-27T13:56:29Z

Can there be any better approach than calling select every second? Like maybe is there a way to make select interrupted by SIGINT/SIGTERM ?

christianparpart · 2018-07-27T19:16:11Z

hey @gumb0. of course there is, by using signalfd() (Linux-only, not OS/X), or with the help with eventfd() in order to break out of the waiting loop (such as select()). The latter can be emulated via pipe() on OS/X too.

However, I didn't wanna be too intrusive for a very first PR in this repo. I can however look into finding a somewhat minimal solution of the latter suggestion I made.

gumb0 · 2018-07-30T17:44:27Z

Let's take a step back, can you describe how exactly do you reproduce it?
I saw this kind of deadlock sometimes, but not every time.

Now when I tried the following steps

start aleth -- test
send some RPC calls
send SIGINT to aleth

accept call is being interrupted and returns -1. And aleth process immediately finishes.

…d a `SIGKILL` to terminate. This was because the UnixSocketServer requires at least one accepting connection *after* SIGTERM/SIGINT has been received, which usually never is the case. This PR implements a trivial workaround by first waiting for up to N msecs (N=1000 for now) for any input to arrive, and if not, it'll just retry, hence, evaluating m_running, resulting in `aleth --test` to properly terminate upon receival of `SIGINT` or `SIGTERM`.

chfast · 2018-07-27T13:42:04Z

+	FD_ZERO(&err);
+	FD_SET(fd, &in);
+	timeval tv { timeoutMillis / 1000, timeoutMillis % 1000 };
+	return select(fd + 1, &in, &out, &err, &tv) > 0;


What fd + does?

chfast · 2018-07-27T13:42:23Z

+		// We do this test before calling accept() in order to gracefully exit
+		// this function when an external thread has set m_running to true
+		// (such as a signal handler in the main thread).
+		if (!waitForReadable(m_socket, 500))


There is not way to add the timeout to accept()?

chfast · 2018-08-01T12:39:57Z

start aleth -- test
send some RPC calls
send SIGINT to aleth

Try without " send some RPC calls".

gumb0 · 2018-08-01T12:45:50Z

Still terminates fine without RPC calls.

Also tried running soltest and SIGINT after that, also aleth is terminated fine.

chfast · 2018-08-01T13:12:45Z

I tried it tool and it terminates quickly. Can it be macOS @christianparpart ?

christianparpart · 2018-08-01T21:24:56Z

So I was facing this issue on "Windows Subsystem for Linux" (which basically is a Linux kernel & ABI re-implementation within the Windows NT kernel).

However, I was trying to reproduce this behavior with a minimal test case, and I believe that I get the same behavior even on Ubuntu 16.04's Linux kernel 4.8.0 with the following test case (link below). So I may still be absolutely brain-dead right now, not finding the bug in my head (or we need this patch)

https://gist.github.com/christianparpart/8b89c9dbfba83478d1308ecc32b9753d

This code is a minimalistic TCP pong server that shares the same behavior this PR tries to fix within aleth's AF_UNIX listener code.

christianparpart · 2018-08-02T00:00:59Z

I digged a little bit into it. it's actually not accept() being cancelled with -1 (EINTR) due to the SIGINT but a -1 (EINVAL) due to the unlink() on the socket. While that one should work on all UNIX'es, I don't think this is a clean solution, as only a side effect of closing a socket while being in a blocking I/O is made use of, which seems unreliable wrt. platform independence (?). I do like your solution (lean, but unfortunately wasn't quite obvious from the beginning), but it doesn't work on my end it seems.

gumb0 · 2018-08-02T17:17:07Z

So we had in place a solution I hoped for, it just seems to not work on exotic systems.

I would prefer to not merge this (it complicates things quite a bit) and resolving this by you @christianparpart submitting a bug report to Microsoft (is this even a bug?)

Otherwise needs some work on better logging / better error reporting / better code formatting.

gumb0 · 2018-08-02T17:18:26Z

~~@chfast this PR somehow is still to develop, not to master~~
nevermind, changed it

christianparpart · 2018-08-03T17:33:46Z

I'd adapt the code to conform to coding style and "codecov" once you say I can proceed (:+1:).

gumb0

Also please apply clang-format to your changes as described in https://github.com/ethereum/cpp-ethereum/blob/master/CONTRIBUTING.md

gumb0 · 2018-08-09T17:56:54Z

 	return path;
 }
+
+/**


We use only // for multiline comments

gumb0 · 2018-08-09T17:57:26Z

+ * @retval true One file descriptor became available.
+ * @retval false Most likely an error occured, or a timeout.
+ */
+static bool waitForReadable(std::initializer_list<int> sockets)


This doesn't need to be declared static as it's in the unnamed namespace

gumb0 · 2018-08-09T18:06:30Z

 {
+	int dummy = 0x90;
+	if (::write(m_exitChannel[1], &dummy, sizeof(dummy)) < 0)
+		cerr << "Failed to notify UNIX domain server loop. " << strerror(errno) << "\n";


Better output it to clog(VerbosityWarn, "rpc"), It will not need \n in the end.

gumb0 · 2018-08-09T18:12:08Z

+	m_exitChannel{-1, -1}
 {
+	if (pipe(m_exitChannel) < 0)
+		abort(); // I don't like. Replace me with throw.


Throw exception here.
Declare it in IpcServerBase.h as

DEV_SIMPLE_EXCEPTION(FailedToCreatePipe);

Then throw here like

BOOST_THROW_EXCEPTION(FailedToCreatePipe());

chfast requested review from axic and gumb0 July 27, 2018 13:12

gumb0 reviewed Jul 27, 2018

View reviewed changes

gumb0 suggested changes Jul 27, 2018

View reviewed changes

Christian Parpart added 2 commits August 1, 2018 12:52

This should fix MacOS build without breaking Linux.

fc1fef4

chfast reviewed Aug 1, 2018

View reviewed changes

UnixSocketServer: optimizes waiting

8cf8cb6

gumb0 changed the base branch from develop to master August 2, 2018 17:18

gumb0 suggested changes Aug 9, 2018

View reviewed changes

Conversation

christianparpart commented Jul 27, 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!

Choose a reason for hiding this comment

Uh oh!

gumb0 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gumb0 commented Jul 27, 2018

Uh oh!

christianparpart commented Jul 27, 2018

Uh oh!

gumb0 commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chfast commented Aug 1, 2018

Uh oh!

gumb0 commented Aug 1, 2018

Uh oh!

chfast commented Aug 1, 2018

Uh oh!

christianparpart commented Aug 1, 2018

Uh oh!

christianparpart commented Aug 2, 2018

Uh oh!

gumb0 commented Aug 2, 2018

Uh oh!

gumb0 commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christianparpart commented Aug 3, 2018

Uh oh!

gumb0 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jul 27, 2018 •

edited

Loading

gumb0 commented Aug 2, 2018 •

edited

Loading