libevent-based http server #2176

str4d · 2017-03-14T05:45:31Z

Cherry-picked from the following upstream PRs:

str4d · 2017-03-14T05:45:56Z

Based on #2175, merge that first.

daira · 2017-03-15T04:25:48Z

How sure are we that this resolves #1680?

bitcartel · 2017-03-15T23:33:16Z

@daira I have a python script which makes zillions of rpc requests. I used it to debug #1680. Can modify to test behaviour under load and with keep-alive option enabled.

bitcartel · 2017-03-22T04:23:46Z

@str4d Just checking, allmention/trace of rpckeepalive has been removed? Are there libevent improvements to backport?

@daira So from empirical testing with local scripts which make repeated calls to the RPC interface, this libevent based server appears faster, but the python scripts will report error: [Errno 99] Cannot assign requested address under load, whereas 1.0.7 currently doesn't do that.

I'm fine with merging this.

str4d · 2017-03-22T20:25:47Z

@bitcartel wrote:

@str4d Just checking, allmention/trace of rpckeepalive has been removed?

Yes.

Are there libevent improvements to backport?

Ooh, yes I have now spotted a couple of additional PRs. I will add them to this PR.

str4d · 2017-03-22T21:18:46Z

@bitcartel it expanded from a "couple" of PRs 😅 Most of the additional PRs are trivial, but we can always split off a few into 1.0.9 if necessary.

str4d · 2017-03-22T21:35:23Z

@zkbot try

zkbot · 2017-03-22T21:35:29Z

⌛ Trying commit 1d3aa37 with merge b0afc4b...

libevent-based http server Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5677 - bitcoin/bitcoin#6695 - bitcoin/bitcoin#6899 - bitcoin/bitcoin#7016 - bitcoin/bitcoin#7964 - bitcoin/bitcoin#8722 - bitcoin/bitcoin#8730 - bitcoin/bitcoin#9073 - bitcoin/bitcoin#9265 - bitcoin/bitcoin#9387 - bitcoin/bitcoin#9471 - bitcoin/bitcoin#9647 - bitcoin/bitcoin#9903 Closes #1593 and #1856.

zkbot · 2017-03-22T22:27:04Z

☀️ Test successful - zcash

…rom upstream." This reverts commit f4404d7.

This option was a temporary workaround, and is no longer necessary with the new web server.

Dropping all use of boost::asio.

Sending a request body with GET request is not valid in HTTP spec, and not compatible with evhttpd.

- *Replace usage of boost::asio with [libevent2](http://libevent.org/)*. boost::asio is not part of C++11, so unlike other boost there is no forwards-compatibility reason to stick with it. Together with zcash#4738 (convert json_spirit to UniValue), this rids Bitcoin Core of the worst offenders with regard to compile-time slowness. - *Replace spit-and-duct-tape http server with evhttp*. Front-end http handling is handled by libevent, a work queue (with configurable depth and parallelism) is used to handle application requests. - *Wrap HTTP request in C++ class*; this makes the application code mostly HTTP-server-neutral - *Refactor RPC to move all http-specific code to a separate file*. Theoreticaly this can allow building without HTTP server but with another RPC backend, e.g. Qt's debug console (currently not implemented) or future RPC mechanisms people may want to use. - *HTTP dispatch mechanism*; services (e.g., RPC, REST) register which URL paths they want to handle. By using a proven, high-performance asynchronous networking library (also used by Tor) and HTTP server, problems such as zcash#5674, zcash#5655, zcash#344 should be avoided. What works? bitcoind, bitcoin-cli, bitcoin-qt. Unit tests and RPC/REST tests pass. The aim for now is everything but SSL support. Configuration options: - `-rpcthreads`: repurposed as "number of work handler threads". Still defaults to 4. - `-rpcworkqueue`: maximum depth of work queue. When this is reached, new requests will return a 500 Internal Error. - `-rpctimeout`: inactivity time, in seconds, after which to disconnect a client. - `-debug=http`: low-level http activity logging

Implement RPCTimerHandler for Qt RPC console, so that `walletpassphrase` works with GUI and `-server=0`. Also simplify HTTPEvent-related code by using boost::function directly. Zcash: QT changes ommitted

@ajweiss

…entBase() Split StartHTTPServer into InitHTTPServer and StartHTTPServer to give clients a window to register their handlers without race conditions. Thanks @ajweiss for figuring this out.

Make it possible to reuse sockets. This is necessary to make the RPC tests work in WINE.

str4d · 2017-03-23T20:08:49Z

Rebased on master. This PR now only contains the libevent commits, but annoyingly GitHub now shows them in date order instead of branch order...

daira

Please respond to comments.

daira · 2017-03-24T02:23:25Z

qa/pull-tester/rpc-tests.sh

+if [ "x${EXEEXT}" = "x.exe" ]; then
+  echo "Win tests currently disabled"
+  exit 0
+fi


Huh. I'd prefer to let them fail on Windows than disable them. Was there any other upstream PR to fix them for Windows?

Nope, they are still disabled by default upstream, and bitcoin/bitcoin#5677 (comment) is referenced as the reason for disabling. In upstream's migration of this script from Bash to Python (bitcoin/bitcoin#6616), an override flag was added, so we will gain that as well once I pull that in.

Yuck. I don't want to hide the fact that these tests are failing on Windows. Please revert the disabling.

daira · 2017-03-24T02:52:13Z

qa/rpc-tests/test_framework/authproxy.py

+            self.__conn.request(method, path, postdata, headers)
+            return self._get_response()
+        except httplib.BadStatusLine as e:
+            if e.line == "''": # if connection was closed, try again


Having read https://bugs.python.org/issue356 , I'm not sure this will actually work in Python 3.5+, because that will throw a ConnectionResetError which, while it does inherit from BadStatusLine, does not necessarily have e.line == "''".

Looks like we also want bitcoin/bitcoin#8139 and bitcoin/bitcoin#8839.

I'm feeling quite smug about spotting that it wouldn't work just from reading the Python issue :-) 👍 on including those PRs.

daira · 2017-03-24T02:55:28Z

src/Makefile.am

  $(LIBZCASH) \
  $(LIBBITCOIN_CRYPTO) \
  $(LIBZCASH_LIBS)

 # bitcoin-cli binary #
 zcash_cli_SOURCES = bitcoin-cli.cpp
-zcash_cli_CPPFLAGS = $(BITCOIN_INCLUDES)
+zcash_cli_CPPFLAGS = $(BITCOIN_INCLUDES) $(EVENT_CFLAGS)


Not also $(EVENT_PTHREADS_CFLAGS)?

Checked with upstream, and they don't have $(EVENT_PTHREADS_CFLAGS) in current master, so unlikely I made a mistake while cherry-picking. I wouldn't expect it to be necessary anyway, as the threading side should only be needed in zcashd.

daira · 2017-03-24T03:00:05Z

src/bitcoin-cli.cpp

+const char *http_errorstring(int code)
+{
+    switch(code) {
+#if LIBEVENT_VERSION_NUMBER >= 0x02010300


We know the version is always at least this, so the conditional compilation is unnecessary.

ACK; I'd like to address this in a separate PR so we can also send it upstream.

daira · 2017-03-24T03:01:54Z

src/bitcoin-cli.cpp

+    }
+}
+
+#if LIBEVENT_VERSION_NUMBER >= 0x02010300


The conditional compilation is unnecessary.

daira · 2017-03-24T03:20:11Z

src/httpserver.cpp

+#ifndef EVENT_LOG_WARN
+// EVENT_LOG_WARN was added in 2.0.19; but before then _EVENT_LOG_WARN existed.
+# define EVENT_LOG_WARN _EVENT_LOG_WARN
+#endif


The conditional block is unnecessary; we should always be using libevent >= 2.0.19.

See #2176 (comment)

daira · 2017-03-24T03:21:03Z

src/httpserver.cpp

+
+    // Redirect libevent's logging to our own log
+    event_set_log_callback(&libevent_log_cb);
+#if LIBEVENT_VERSION_NUMBER >= 0x02010100


Conditional compilation unnecessary.

See #2176 (comment)

daira · 2017-03-24T03:27:17Z

src/httpserver.cpp

+            break;
+    if (i != iend)
+    {
+        LogPrint("http", "Unregistering HTTP handler for %s (exactmath %d)\n", prefix, exactMatch);


Typo: s/exactmath/exactmatch/

I'll pull in bitcoin/bitcoin#6640.

daira · 2017-03-24T03:29:55Z

src/httpserver.h

+     * nStatus is the HTTP status code to send.
+     * strReply is the body of the reply. Keep it empty to send a standard message.
+     *
+     * @note Can be called only once. As this will give the request back to the


This really calls out for session types. Oh well.

Is there a ticket we can open, or is this a missing language feature?

Missing language feature. I was just griping about C++'s inexpressiveness.

daira · 2017-03-24T03:36:29Z

src/rpcserver.h

+     * RPC will call the function to create a timer that will call func in *millis* milliseconds.
+     * @note As the RPC mechanism is backend-neutral, it can use different implementations of timers.
+     * This is needed to cope with the case in which there is no HTTP server, but
+     * only GUI RPC console, and to break the dependency of pcserver on httprpc.


Typo: s/pcserver/rpcserver/

... but not yet in trivial tree

This is necessary on FreeBSD and MacOSX, at least. See bitcoin/bitcoin#8834 (comment)

str4d · 2017-03-24T05:47:45Z

@daira comments responded to, and mostly addressed (just waiting on #2176 (comment) and #2176 (comment)).

daira · 2017-03-25T03:34:44Z

Please revert the disabling of RPC tests on Windows, and fix the "pcserver" typo. Otherwise utACK.

This reverts commit 7fd5d4e. We need to un-revert this before pulling in bitcoin/bitcoin#6616

ebfull · 2017-03-25T04:45:14Z

This is a fantastic cherry-pick, I'm ACK. Nice and clean.

bitcartel · 2017-03-25T04:50:49Z

ACK @zkbot r+

zkbot · 2017-03-25T04:50:50Z

📌 Commit 3da13e8 has been approved by bitcartel

zkbot · 2017-03-25T04:50:54Z

⌛ Testing commit 3da13e8 with merge f9f4866...

libevent-based http server Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5677 - bitcoin/bitcoin#6695 - bitcoin/bitcoin#6899 - bitcoin/bitcoin#7016 - bitcoin/bitcoin#7964 - bitcoin/bitcoin#8722 - bitcoin/bitcoin#8730 - bitcoin/bitcoin#9073 - bitcoin/bitcoin#9265 - bitcoin/bitcoin#9387 - bitcoin/bitcoin#9471 - bitcoin/bitcoin#9647 - bitcoin/bitcoin#9903 - bitcoin/bitcoin#6640 - bitcoin/bitcoin#8139 - bitcoin/bitcoin#8839 Closes #1593 and #1856.

zkbot · 2017-03-25T05:48:47Z

☀️ Test successful - zcash

daira · 2017-03-25T15:15:10Z

Post-hoc utACK for the last three commits.

str4d added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-rpc-interface Area: RPC interface C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. labels Mar 14, 2017

str4d added this to the 1.0.8 milestone Mar 14, 2017

str4d mentioned this pull request Mar 14, 2017

Tor ephemeral hidden services #2177

Merged

str4d force-pushed the 1593-libevent branch 3 times, most recently from a93d092 to 5779978 Compare March 22, 2017 20:47

str4d and others added 12 commits March 24, 2017 09:03

Revert "Closes zcash#1680, temporary fix for rpc deadlock inherited f…

bec25c9

…rom upstream." This reverts commit f4404d7.

doc: remove documentation for rpcssl

b6fbda0

qa: Remove -rpckeepalive tests from httpbasics

14dcc6a

This option was a temporary workaround, and is no longer necessary with the new web server.

Remove rpc_boostasiotocnetaddr test

72f5242

Dropping all use of boost::asio.

libevent: add depends

505b30f

build: build-system changes for libevent

df377ca

tests: GET requests cannot have request body, use POST in rest.py

cc14ac4

Sending a request body with GET request is not valid in HTTP spec, and not compatible with evhttpd.

Implement RPCTimerHandler for Qt RPC console

858afa1

Implement RPCTimerHandler for Qt RPC console, so that `walletpassphrase` works with GUI and `-server=0`. Also simplify HTTPEvent-related code by using boost::function directly. Zcash: QT changes ommitted

Document options for new HTTP/RPC server in --help

9fb5b94

Fix race condition between starting HTTP server thread and setting Ev…

116503c

…entBase() Split StartHTTPServer into InitHTTPServer and StartHTTPServer to give clients a window to register their handlers without race conditions. Thanks @ajweiss for figuring this out.

libevent: Windows reuseaddr workaround in depends

91295c4

Make it possible to reuse sockets. This is necessary to make the RPC tests work in WINE.

daira requested changes Mar 24, 2017

View reviewed changes

str4d mentioned this pull request Mar 24, 2017

Add AMQP 1.0 support via Apache Qpid Proton C++ API 0.17.0 #2189

Merged

paveljanik and others added 3 commits March 24, 2017 18:42

[TRIVIAL] Fix typo: exactmath -> exactmatch

17694e4

... but not yet in trivial tree

Fix interrupted HTTP RPC connection workaround for Python 3.5+

2edad86

test: Avoid ConnectionResetErrors during RPC tests

e93a3e1

This is necessary on FreeBSD and MacOSX, at least. See bitcoin/bitcoin#8834 (comment)

daira approved these changes Mar 25, 2017

View reviewed changes

str4d added 3 commits March 25, 2017 17:37

Revert "Revert "rpc-tests: re-enable rpc-tests for Windows""

47855b5

This reverts commit 7fd5d4e. We need to un-revert this before pulling in bitcoin/bitcoin#6616

Wrap error string

206e2b9

Fix typo

3da13e8

str4d mentioned this pull request Mar 25, 2017

Remove libevent conditional compilation #2207

Open

str4d mentioned this pull request Mar 25, 2017

Ensure externally-provided strings in LogPrint calls are sanitized #2208

Open

zkbot merged commit 3da13e8 into zcash:master Mar 25, 2017

zkbot mentioned this pull request Mar 25, 2017

Upgrade googletest to 1.8.0 #1437

Closed

str4d deleted the 1593-libevent branch March 25, 2017 07:16

str4d mentioned this pull request Mar 25, 2017

Bitcoin Core 0.12.0 #2074

Open

daira mentioned this pull request Mar 26, 2017

Re-enable HTTP persistent connections (keep-alive) for RPC #1856

Closed

str4d mentioned this pull request Apr 6, 2017

Fix Buildbot hanging during performance benchmarks #2238

Closed

str4d mentioned this pull request Jul 12, 2017

RPC 401 Unauthorized After Upgrade to version 1000950 #2444

Closed

dagurval mentioned this pull request Jul 12, 2017

libevent-based http server bitcoinxt/bitcoinxt#172

Merged

str4d mentioned this pull request Jul 22, 2021

How fix error TLS handshake on localhost for developing ? #5248

Closed

libevent-based http server #2176

libevent-based http server #2176

Conversation

str4d commented Mar 14, 2017 • edited Loading

str4d commented Mar 14, 2017

daira commented Mar 15, 2017

bitcartel commented Mar 15, 2017

bitcartel commented Mar 22, 2017 • edited Loading

str4d commented Mar 22, 2017

str4d commented Mar 22, 2017

str4d commented Mar 22, 2017

zkbot commented Mar 22, 2017

zkbot commented Mar 22, 2017

str4d commented Mar 23, 2017

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Mar 25, 2017 • edited Loading

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

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

daira Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Mar 24, 2017

daira commented Mar 25, 2017

ebfull commented Mar 25, 2017

bitcartel commented Mar 25, 2017

zkbot commented Mar 25, 2017

zkbot commented Mar 25, 2017

zkbot commented Mar 25, 2017

daira commented Mar 25, 2017

str4d commented Mar 14, 2017 •

edited

Loading

bitcartel commented Mar 22, 2017 •

edited

Loading

daira Mar 25, 2017 •

edited

Loading

daira Mar 25, 2017 •

edited

Loading

daira Mar 25, 2017 •

edited

Loading