Cli port input #736

beidouz · 2018-11-29T20:27:44Z

Notice

It is not allowed to submit your PR to the master branch directly, please submit your PR to the master-pre-merge branch.

Description

Please include a brief summary of the change that this pull request proposes. Include any relevant motivation and context. List any dependencies required for this change.

Allow running the kernel with a given port number.

Fixes Issue #725 .

Type of change

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

Testing

Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.

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.

aion-kelvin

Hey, this looks mostly good. Thanks for the thorough tests. I have a two small suggestions below.

I noticed that if you set the port value to something invalid, i.e. empty string, or a number that is invalid, the kernel starts up, prints an error message, then continues running. I noticed we do this a lot in the kernel -- serious errors often don't cause the kernel to stop. I think failing to set the port number should not allow the kernel to start up. Any thoughts on this @arajasek @AlexandraRoatis @AionJayT ?

aion-kelvin · 2018-11-29T23:16:38Z

modAionImpl/src/org/aion/zero/impl/config/CfgAion.java

@@ -354,7 +354,7 @@ public void toXML(final String[] args, File file) {
                    override = true;
                    String[] subArgsArr = arg.replace("--p2p=", "").split(",");
                    if (subArgsArr.length == 2) {
-                        this.getNet().getP2p().setIp(subArgsArr[0]);
+                        if (!subArgsArr[0].equals("")) this.getNet().getP2p().setIp(subArgsArr[0]);


Could you run the code formatter with our formatting guidelines? I think we put the code of the if inside { } or at least on a different line.

will do, thanks!

still missing the correct formatting

if (cond) { cmd; }

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

AlexandraRoatis · 2018-11-30T18:33:09Z

@aion-kelvin I agree in the general case, but here I think we should opt for setting the port to the default 30303 value when given an incorrect value as input.

aion-kelvin · 2018-11-30T19:14:41Z

@aion-kelvin I agree in the general case, but here I think we should opt for setting the port to the default 30303 value when given an incorrect value as input.

I'm ok with that. @beidouz could we implement it this way? Also, make sure it doesn't write to the config file if the port is detected to be not valid. It should also print an error/warning.

AionJayT

LGTM

AlexandraRoatis · 2018-12-04T22:09:01Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

@@ -231,6 +231,20 @@ public ReturnType call(final String[] args, Cfg cfg) {

            // 5. options that can be influenced by the -d and -n arguments

+            if (options.getPort() != null) {
+                int port_number = Integer.parseInt(options.getPort());


parseInt throws an exception when given a string that is not a number. This should be handled without crashing the kernel. For example ./aion.sh -p test will show a NumberFormatException instead of properly notifying the user that an incorrect value has been given as parameter.

To solve this issue you can surround the statement with a try-catch and print the adequate message inside the catch, or set a boolean flag to true and check for the flag in the if statement below.

I've noticed a tangentially-related issue that I think we should fix in this review. Linux can be configured to require root permissions for opening certain ports. By default, ports <= 1024 are need root, but it could be configured to be anything. Example:

sergiu@aion-XPS-8930:~/repos/aion2$ ./aion.sh -p 140 [...] 18-12-05 14:28:41.107 INFO GEN [main]: loaded block <num=24, root=2c0448b8... l=32> 18-12-05 14:28:41.171 ERROR P2P [main]: tcp-server-socket-exception. java.net.SocketException: Permission denied at java.base/sun.nio.ch.Net.bind0(Native Method) at java.base/sun.nio.ch.Net.bind(Net.java:461) at java.base/sun.nio.ch.Net.bind(Net.java:453) at java.base/sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:227) at java.base/sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:80) at org.aion.p2p.impl1.P2pMgr.run(P2pMgr.java:194) at org.aion.zero.impl.AionHub.initializeHub(AionHub.java:205) at org.aion.zero.impl.AionHub.<init>(AionHub.java:125) at org.aion.zero.impl.blockchain.AionImpl.<init>(AionImpl.java:77) at org.aion.zero.impl.blockchain.AionImpl.<init>(AionImpl.java:55) at org.aion.zero.impl.blockchain.AionImpl$Holder.<clinit>(AionImpl.java:68) at org.aion.zero.impl.blockchain.AionImpl.inst(AionImpl.java:72) at org.aion.zero.impl.blockchain.AionFactory.create(AionFactory.java:34) at org.aion.Aion.main(Aion.java:223) 18-12-05 14:28:41.175 INFO GEN [main]: <node-started endpoint=p2p://32b3fee1-a64d-4b05-baca-0f7101394b63@0.0.0.0:140> 18-12-05 14:28:41.178 INFO CONS [main]: <sealing-disabled>

Then the kernel continues running. I don't think the input validation of our code should check for this (there's other things we can't anticipate i.e. they specified a port that's already in use). Instead, can we put a try/catch around the code that opens the port and print out an error if the port can't be opened?

AlexandraRoatis · 2018-12-04T22:20:06Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

+                                    + ", switching to the current port configuration: \n");
+                } else {
+                    // update port in the config file
+                    cfg.toXML(new String[] {"--p2p=" + "," + options.getPort()}, configFile);


This line will modify the initial config file. You can verify that config/mainnet/config.xml will have the value of the port. It also ends up writing the id in the initial file, which is not the desired outcome. To avoid these side-effects you can set the port value inside the cfg object and set the overwrite flag to true. Writing to file will be triggered correctly below due to the overwrite flag.

Additionally, you can see in the tests (e.g. options -p 12345) that the config file is overwritten twice (not necessary).

AlexandraRoatis · 2018-12-04T22:22:30Z

modAionImpl/src/org/aion/zero/impl/config/CfgAion.java

@@ -354,7 +354,7 @@ public void toXML(final String[] args, File file) {
                    override = true;
                    String[] subArgsArr = arg.replace("--p2p=", "").split(",");
                    if (subArgsArr.length == 2) {
-                        this.getNet().getP2p().setIp(subArgsArr[0]);
+                        if (!subArgsArr[0].equals("")) this.getNet().getP2p().setIp(subArgsArr[0]);


still missing the correct formatting

if (cond) { cmd; }

AlexandraRoatis · 2018-12-04T22:40:58Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

+                    System.out.println(
+                            "\nport out of range: "
+                                    + port_number
+                                    + ", switching to the current port configuration: \n");


please add here what port will be used

+1 for add default port

AlexandraRoatis · 2018-12-04T22:49:49Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

@@ -231,6 +231,20 @@ public ReturnType call(final String[] args, Cfg cfg) {

            // 5. options that can be influenced by the -d and -n arguments

+            if (options.getPort() != null) {


I think this option can work with -c, --config as well. The if (options.getPort() != null) {...} statement can be moved above the if (options.getConfig() != null) {...} statement. This will require adding more test cases as well.

AlexandraRoatis · 2018-12-04T22:53:33Z

modAionImpl/test/org/aion/zero/impl/cli/CliTest.java

+                    new Object[] {
+                        new String[] {opPort, "-12345"}, RUN, expPathOnError, expPortOnError
+                    });
+            // with testing port number


we also need here a string value for the port that is not a number, e.g. test

AlexandraRoatis · 2018-12-04T22:57:39Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

@@ -861,6 +875,9 @@ TaskPriority getBreakingTaskPriority(Arguments options) {
            if (options.getDirectory() != null) {
                skippedTasks.add("--datadir");
            }
+            if (options.getPort() != null) {


Please add some tests that use the port option in the CliTest.testCheckArguments as well.

+1 for adding some tests for the input validation

AlexandraRoatis · 2018-12-04T22:59:00Z

modAionImpl/test/org/aion/zero/impl/cli/CliTest.java

+        assertThat(cfg.getKeystoreDir()).isEqualTo(new File(expectedPath, "keystore"));
+        // test port is updated
+        assertThat(Integer.toString(cfg.getNet().getP2p().getPort())).isEqualTo(expectedPort);
+


This method should also check that the initial config file port is unchanged.

aion-kelvin

Hey, looks good so far. After my initial review, I realized there's one more thing we should do to make our error handling for ports better (details below).

aion-kelvin · 2018-12-05T17:49:33Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

@@ -231,6 +231,20 @@ public ReturnType call(final String[] args, Cfg cfg) {

            // 5. options that can be influenced by the -d and -n arguments

+            if (options.getPort() != null) {
+                int port_number = Integer.parseInt(options.getPort());


style: port_number should be portNumber

aion-kelvin · 2018-12-05T17:49:53Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

+                    System.out.println(
+                            "\nport out of range: "
+                                    + port_number
+                                    + ", switching to the current port configuration: \n");


+1 for add default port

aion-kelvin · 2018-12-05T17:50:46Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

@@ -861,6 +875,9 @@ TaskPriority getBreakingTaskPriority(Arguments options) {
            if (options.getDirectory() != null) {
                skippedTasks.add("--datadir");
            }
+            if (options.getPort() != null) {


+1 for adding some tests for the input validation

aion-kelvin · 2018-12-05T19:37:09Z

modAionImpl/src/org/aion/zero/impl/cli/Cli.java

@@ -231,6 +231,20 @@ public ReturnType call(final String[] args, Cfg cfg) {

            // 5. options that can be influenced by the -d and -n arguments

+            if (options.getPort() != null) {
+                int port_number = Integer.parseInt(options.getPort());


I've noticed a tangentially-related issue that I think we should fix in this review. Linux can be configured to require root permissions for opening certain ports. By default, ports <= 1024 are need root, but it could be configured to be anything. Example:

sergiu@aion-XPS-8930:~/repos/aion2$ ./aion.sh -p 140 [...] 18-12-05 14:28:41.107 INFO GEN [main]: loaded block <num=24, root=2c0448b8... l=32> 18-12-05 14:28:41.171 ERROR P2P [main]: tcp-server-socket-exception. java.net.SocketException: Permission denied at java.base/sun.nio.ch.Net.bind0(Native Method) at java.base/sun.nio.ch.Net.bind(Net.java:461) at java.base/sun.nio.ch.Net.bind(Net.java:453) at java.base/sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:227) at java.base/sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:80) at org.aion.p2p.impl1.P2pMgr.run(P2pMgr.java:194) at org.aion.zero.impl.AionHub.initializeHub(AionHub.java:205) at org.aion.zero.impl.AionHub.<init>(AionHub.java:125) at org.aion.zero.impl.blockchain.AionImpl.<init>(AionImpl.java:77) at org.aion.zero.impl.blockchain.AionImpl.<init>(AionImpl.java:55) at org.aion.zero.impl.blockchain.AionImpl$Holder.<clinit>(AionImpl.java:68) at org.aion.zero.impl.blockchain.AionImpl.inst(AionImpl.java:72) at org.aion.zero.impl.blockchain.AionFactory.create(AionFactory.java:34) at org.aion.Aion.main(Aion.java:223) 18-12-05 14:28:41.175 INFO GEN [main]: <node-started endpoint=p2p://32b3fee1-a64d-4b05-baca-0f7101394b63@0.0.0.0:140> 18-12-05 14:28:41.178 INFO CONS [main]: <sealing-disabled>

Then the kernel continues running. I don't think the input validation of our code should check for this (there's other things we can't anticipate i.e. they specified a port that's already in use). Instead, can we put a try/catch around the code that opens the port and print out an error if the port can't be opened?

AlexandraRoatis

Good job!

aion-kelvin

Hey, thanks for your work.

One observation: if I use ./aion.sh -p 140 it will pass the basic checks but then fail to open the socket. However, after passing the basic checks, the config.xml is already updated. If no other reviewers have a problem with this, I'm ok with leaving this the way it is because I don't think the P2pMgr (the thing opening socket, which is the place you catch the socket opening error case) should go edit a config file to set it back.

Also have one minor suggestion for formatting below.

Other than that, looks good to me!

aion-kelvin · 2018-12-11T22:18:55Z

modAionImpl/src/org/aion/zero/impl/cli/Arguments.java

@@ -240,6 +245,8 @@ public String getDirectory() {
        return directory;
    }

+    public String getPort() { return port; }


public String getPort() {
return port;
}

beidouz added this to the 0.3.3 milestone Nov 29, 2018

beidouz self-assigned this Nov 29, 2018

beidouz requested review from arajasek, AlexandraRoatis, AionJayT and aion-kelvin November 29, 2018 20:27

beidouz added the feature label Nov 29, 2018

aion-kelvin suggested changes Nov 29, 2018

View reviewed changes

beidouz force-pushed the cli-port-input branch 2 times, most recently from acf02cd to 68cd950 Compare December 3, 2018 22:06

AionJayT approved these changes Dec 4, 2018

View reviewed changes

AlexandraRoatis suggested changes Dec 4, 2018

View reviewed changes

aion-kelvin suggested changes Dec 5, 2018

View reviewed changes

beidouz force-pushed the cli-port-input branch 3 times, most recently from 6432825 to 0d22910 Compare December 7, 2018 23:00

AlexandraRoatis approved these changes Dec 11, 2018

View reviewed changes

aion-kelvin approved these changes Dec 11, 2018

View reviewed changes

beidouz added 8 commits December 11, 2018 18:05

add port number as CLI input option

2e9c152

add test: port number as CLI input

4df66c5

add cli port validation & update tests

06ded6a

cli port input must be int; initial config should not be modified

0c759d7

update cli port option can be combined with --config; add tests

db2884f

print error message when port cannot be opened

d5c9a83

add a cli test for port&Info input

7ffe6b2

cli change minor reformat

12a9aa7

beidouz force-pushed the cli-port-input branch from 8db24c9 to 12a9aa7 Compare December 11, 2018 23:06

beidouz mentioned this pull request Dec 11, 2018

Socket connection failure with Cli port input #751

Closed

AlexandraRoatis merged commit 3fff193 into master-pre-merge Dec 12, 2018

AionJayT deleted the cli-port-input branch January 9, 2019 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cli port input #736

Cli port input #736

beidouz commented Nov 29, 2018

aion-kelvin left a comment

aion-kelvin Nov 29, 2018

beidouz Nov 30, 2018

AlexandraRoatis Dec 4, 2018

AlexandraRoatis commented Nov 30, 2018

aion-kelvin commented Nov 30, 2018

AionJayT left a comment

AlexandraRoatis Dec 4, 2018

aion-kelvin Dec 5, 2018

AlexandraRoatis Dec 4, 2018

AlexandraRoatis Dec 4, 2018

AlexandraRoatis Dec 4, 2018

aion-kelvin Dec 5, 2018

AlexandraRoatis Dec 4, 2018

AlexandraRoatis Dec 4, 2018

beidouz Dec 5, 2018

AlexandraRoatis Dec 4, 2018

aion-kelvin Dec 5, 2018

AlexandraRoatis Dec 4, 2018

aion-kelvin left a comment

aion-kelvin Dec 5, 2018

beidouz Dec 5, 2018

aion-kelvin Dec 5, 2018

aion-kelvin Dec 5, 2018

aion-kelvin Dec 5, 2018

AlexandraRoatis left a comment

aion-kelvin left a comment

aion-kelvin Dec 11, 2018

		@@ -231,6 +231,20 @@ public ReturnType call(final String[] args, Cfg cfg) {

		// 5. options that can be influenced by the -d and -n arguments

		if (options.getPort() != null) {

Cli port input #736

Cli port input #736

Conversation

beidouz commented Nov 29, 2018

Notice

Description

Type of change

Testing

Verification

aion-kelvin 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

AlexandraRoatis commented Nov 30, 2018

aion-kelvin commented Nov 30, 2018

AionJayT 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

aion-kelvin 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

AlexandraRoatis left a comment

Choose a reason for hiding this comment

aion-kelvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment