Rpc Server Reliability Upgrades #619

ali-sharif · 2018-08-27T17:18:03Z

1. Description

Note: PR contingent upon @AlexandraRoatis testing and green light the usage of Repository.getSnapshotTo(StateRoot) in ApiWeb3Aion.java.

Bug Fix: Fixes inapproprate use of RequestBufferingHandler which was causing the following error (and subsequent server shutdown) from Undertow:

18-08-25 10:34:06.554 ERROR io.undertow.request [XNIO-1 I/O-1]: UT005071: Undertow request failed HttpServerExchange{ POST / request {Accept=[*/*], Postman-Token=[], Accept-Language=[zh-CN,zh;q=0.9], Cache-Control=[no-cache], Accept-Encoding=[gzip, deflate], Origin=[chrome-extension://fhbjgbiflinjbdggehcddcbncdddomop], User-Agent=[Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36], Connection=[keep-alive], Content-Length=[341], Content-Type=[application/json], Host=[]} response {}}
java.lang.IllegalStateException: UT000126: Attempted to do blocking IO from the IO thread. This is prohibited as it may result in deadlocks
 at io.undertow.io.UndertowInputStream.read(UndertowInputStream.java:84)
 at io.undertow.io.BlockingReceiverImpl.receiveFullString(BlockingReceiverImpl.java:124)
 at io.undertow.io.BlockingReceiverImpl.receiveFullString(BlockingReceiverImpl.java:76)
 at org.aion.api.server.http.undertow.UndertowRpcServer.lambda$handleRequest$1(Unknown Source)
 at io.undertow.server.Connectors.executeRootHandler(Connectors.java:360)
 at io.undertow.server.handlers.RequestBufferingHandler$1.handleEvent(RequestBufferingHandler.java:99)
 at io.undertow.server.handlers.RequestBufferingHandler$1.handleEvent(RequestBufferingHandler.java:78)
 at org.xnio.ChannelListeners.invokeChannelListener(ChannelListeners.java:92)
 at io.undertow.channels.DetachableStreamSourceChannel$SetterDelegatingListener.handleEvent(DetachableStreamSourceChannel.java:231)
 at io.undertow.channels.DetachableStreamSourceChannel$SetterDelegatingListener.handleEvent(DetachableStreamSourceChannel.java:218)
 at org.xnio.ChannelListeners.invokeChannelListener(ChannelListeners.java:92)
 at org.xnio.conduits.ReadReadyHandler$ChannelListenerHandler.readReady(ReadReadyHandler.java:66)
 at org.xnio.nio.NioSocketConduit.handleReady(NioSocketConduit.java:88)
 at org.xnio.nio.WorkerThread.run(WorkerThread.java:561)

Added configuration options to set worker-threads, io-threads & request-queue-size in the configuration.
Fixed mis-reporting of rpc-request timing by delegating request timing to Guava.
Generated extensive documentation for users of the RPC server: https://github.com/ali-sharif/aion/wiki/JSON-RPC-API-Documentation (will be committed to the Aion wiki when this PR goes through)
Re-enabled retrieving state-snapshots at particular block numbers (ie. get account balance at particular block number, etc) via the default block parameter for the functions:
- eth_getBalance
- eth_getCode
- eth_getTransactionCount
- eth_getStorageAt
- eth_call
Made NanoHttpd request queue unbounded due to no good graceful request close API exposed by the library at that point in request lifecycle (only ungraceful socket-shutdown available at that point in request lifecycle).

2. Type of change

Insert x into the following checkboxes to confirm (eg. [x]):

3. Testing

Added testcase to javascript rpc-server testbench in aion_qa to test changes to eth_getBalance
Manually tested new configuration settings.
Load test result extract below (detailed report: https://github.com/ali-sharif/aion/wiki/JSON-RPC-API-Load-Test-Details)

3.1 Resource Constrained Test

worker threads = 4
io threads = 1 (undertow only)
request queue size = 200 (undertow only)

	c=1, r=1000	c=20, r=1000	c=500, r=1000
Undertow	812.38 / 100%	1134.18 / 100%	908.69 / 100%
NanoHttpd	22.58 / 100%	55.69 / 98%	84.62 / 55% **

Legend: [avg requests per sec processed / success rate %]

** requests fail due to a combination of resource-busy due to blocking architecture & aggresive (2s) client timeout, but RPC server survives

4. Verification

Insert x into the following checkboxes to confirm (eg. [x]):

I have self-reviewed my own code and conformed to the style guidelines of this project.
New and existing tests pass locally with my changes.
I have added tests for my fix or feature.
I have made appropriate changes to the corresponding documentation.
My code generates no new warnings.
Any dependent changes have been made.

AlexandraRoatis

minor comments so far, still reviewing

AlexandraRoatis · 2018-08-27T18:36:58Z

modApiServer/src/org/aion/api/server/http/nano/NanoRpcServer.java

-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.ThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
+import java.util.Optional;


AlexandraRoatis · 2018-08-27T18:37:46Z

modApiServer/src/org/aion/api/server/http/nano/NanoRpcServer.java

@@ -93,24 +90,42 @@ public void makeSecure() throws Exception {
    @Override
    public void start() {
        try {
-            // default to 1 thread to minimize resource consumption by nano http
-            int tCount = 1;
+            /**


should be comment /* not java doc /**

AlexandraRoatis · 2018-08-27T18:40:13Z

modApiServer/src/org/aion/api/server/http/undertow/AionUndertowRootHandler.java

+import org.slf4j.Logger;
+
+import java.util.Map;
+import java.util.Optional;


a number of imports are not used

AlexandraRoatis · 2018-08-27T21:08:51Z

modApiServer/src/org/aion/api/server/http/undertow/AionUndertowRpcHandler.java

+    }
+
+    @Override
+    public void handleRequest(HttpServerExchange exchange) throws Exception {


I don't see any exception being thrown here

AlexandraRoatis · 2018-08-27T21:16:40Z

modApiServer/src/org/aion/api/server/http/undertow/UndertowRpcServer.java

@@ -26,16 +16,17 @@
 import java.util.Map;

 public class UndertowRpcServer extends RpcServer {
+


The RpcServer class could include more of the common functionality between UndertowRpcServer and NanoRpcServer.

I don't think there are any more commonalities that could be easily pulled up to the base class without expending some effort generifying and refactoring, which would unneccessarily complicate the implementation.

If you had some specific ideas, I'm all ears :) (but I think the implementations are simple enough and don't thing we have alot to gain by refactoring this class heirarchy).

I was thinking that the LOG and CORS_HEADERS can be placed in the abstract class and perhaps the log debug printout with the options display.

AlexandraRoatis · 2018-08-27T21:22:05Z

modMcf/src/org/aion/mcf/config/CfgApiRpc.java

@@ -115,25 +123,63 @@ public void fromXML(final XMLStreamReader sr) throws XMLStreamException {
                                e.printStackTrace();
                            }
                            break;
-                        case "threads":
+                        case "worker-threads": {


this may make old config files incompatible

yes, that was the point. Initially, I had both threads and worker-threads map to the same case, but then I didn't want the user the explicitly set the worker-thread count unless they really knew what they were doing, so I ended up removing the threads options altogether. The user would have to read through the RPC config wiki to find the appropriate option to constrain thread count.

@ali-sharif do you tested the config file capabilities? Meaning the old version will not block the kernel running.

Yes, tested that if threads is declared in the config file, the kernel just ignores it and uses the default value, which was the behaviour I wanted to enforce. In general, if an xml node is found in the config but not in the mapper, it's just ignored.

Is there a place where you document these breaking changes for things like config files? If I wouldn't have looked into the pull request, I don't know where I could've find this information. Also, given the fact that pools are very dependent on this configuration, we really need to have this information into the open.

just to confirm, no breaking changes to the config. we spent a lot of time writing very detailed documentation on the RPC server https://github.com/aionnetwork/aion/wiki/JSON-RPC-API-Docs, which should have been communicated to the pools (we will try to remedy that soon)

AlexandraRoatis · 2018-08-27T21:25:36Z

modMcf/src/org/aion/mcf/config/CfgApiRpc.java

@@ -171,7 +217,7 @@ String toXML() {
            xmlWriter.writeComment("boolean, enable/disable cross origin requests (browser enforced)");
            xmlWriter.writeCharacters("\r\n\t\t\t");
            xmlWriter.writeStartElement("cors-enabled");
-            xmlWriter.writeCharacters(String.valueOf(this.getCorsEnabled()));
+            xmlWriter.writeCharacters(String.valueOf(this.isCorsEnabled()));


Why weren't the other configuration options added here?

As is stated in the RPC docs, the other settings are for advanced users and putting them in the config would just give users opportunity to tweak those settings without really knowing what those settings do.

the advanced config settings for the rpc-server will disappear after executing./aion.sh -c
Is that an expected behavior?

AionJayT

LGTM, one comment need to confirm.

rpc server reliability upgrades

b34fcca

ali-sharif requested review from AionJayT and arajasek August 27, 2018 17:18

ali-sharif mentioned this pull request Aug 27, 2018

RPC Server Reliability Upgrades #618

Closed

12 tasks

AlexandraRoatis mentioned this pull request Aug 27, 2018

Small test for repository snapshot #620

Merged

12 tasks

AlexandraRoatis reviewed Aug 27, 2018

View reviewed changes

ali-sharif added 2 commits August 27, 2018 14:55

style and import fixes

eface81

make makeSecure() access private in NanoRpcServer.java

6751f1b

AlexandraRoatis suggested changes Aug 27, 2018

View reviewed changes

removed unnecessary throws clause from AionUndertowRpcHandler.java

c233761

AlexandraRoatis approved these changes Aug 28, 2018

View reviewed changes

AionJayT reviewed Aug 28, 2018

View reviewed changes

AionJayT approved these changes Aug 28, 2018

View reviewed changes

added link to rpc configuration wiki in config file

3629342

AionJayT merged commit 7cabf83 into master Aug 28, 2018

AionJayT deleted the rpc-upgrades branch September 7, 2018 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rpc Server Reliability Upgrades #619

Rpc Server Reliability Upgrades #619

ali-sharif commented Aug 27, 2018

AlexandraRoatis left a comment

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018

AlexandraRoatis Aug 28, 2018

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018

AionJayT Aug 28, 2018

ali-sharif Aug 28, 2018

alinturbut Aug 31, 2018

ali-sharif Aug 31, 2018

AlexandraRoatis Aug 27, 2018

ali-sharif Aug 27, 2018 •

edited

Loading

AionJayT Aug 28, 2018

AionJayT left a comment

		@@ -26,16 +16,17 @@
		import java.util.Map;

		public class UndertowRpcServer extends RpcServer {

Rpc Server Reliability Upgrades #619

Rpc Server Reliability Upgrades #619

Conversation

ali-sharif commented Aug 27, 2018

1. Description

2. Type of change

3. Testing

3.1 Resource Constrained Test

4. Verification

AlexandraRoatis 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

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

ali-sharif Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AionJayT left a comment

Choose a reason for hiding this comment

ali-sharif Aug 27, 2018 •

edited

Loading