Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC server is moved to a new process #1874

Merged
merged 29 commits into from Apr 21, 2019

Conversation

@wezrule
Copy link
Collaborator

commented Mar 29, 2019

This solves #1827

The RPC server has now been moved to a new executable. If rpc_enable is set to true a new rpc_path config option will be added which points to the nano_rpc executable, and this will be launched as a child process of the nano_node executable. When rpc_enable is false the RPC process will not be started, however it can be started manually with nano_rpc --daemon either before or after nano_node --daemon or starting the gui wallet. In the event that the node crashes or needs upgrading but the RPC process doesn't, the RPC process can try and reconnect to the node on the next RPC command fired so it does not require restarting as well.

There are no API changes required as the current JSON requests are simply passed via IPC transport mechanism to be handled inside the node now. This required removing anything related to the node from the RPC process (which has been done largely in other PRs) as they are now separate services and don't know about each other (theoretically).

A new json file has been created (rpc_config.json) for it and upon upgrade the settings from config.json are migrated. enable_sign_hash and max_work_generate_difficulty have been moved to the ipc config as the node itself needs to know about it deep in the logic. log_rpc has currently just been removed and no explicit logging config options (can be added in the future if required) have been added to the RPC server, it now has a log_rpc.*.log file in similar format to the existing log file.

A few IPC connections are made, defaulting to 8 on the live network which is user configurable with the rpc_config.json option num_ipc_connections. The new class rpc_request_processor queues up requests and dispatches them to any available connections.

When testing:
Try the stop RPC command, it should close down both nano_node and nano_rpc processes, whether they are separate or as a child process with rpc_enable. If the RPC server is started successfuly and the node goes down, restarting the node should allow the RPC server to connect to it again when a new RPC command is issued.
Try the RPC server with the gui wallet.
Try config changes and upgrades with both node and gui.
Check that the new log file is working as expected.

There are now 3 different ways to run the RPC server:
In process

  • rpc_enable = true
  • rpc_in_process = true (default)

Child process

  • rpc_enable = true
  • rpc_path = path to nano_rpc
  • rpc_in_process = false
  • ipc -> tcp -> enable = true
  • ipc -> tcp -> port == ipc_port of rpc_config.json

Out of process
As above but rpc_enable = false

@wezrule wezrule added this to the V19.0 milestone Mar 29, 2019

@wezrule wezrule self-assigned this Mar 29, 2019

@wezrule wezrule requested review from cryptocode and clemahieu Mar 29, 2019

@wezrule wezrule added this to CP3 (2019-04-10) in V19 Mar 29, 2019

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2019

Seems like I'm hitting boostorg/process#55 on macOS/latest clang/boost 1.69

@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2019

Seems like I'm hitting boostorg/process#55 on macOS/latest clang/boost 1.69

Well that's annoying. I was able to reproduce it (works ok with 1.68), according to that link it will be fixed in 1.70. There is actually a std::system () function but it is only synchronous and the standard doesn't really have a concept of processes yet, so there's no way to explicitly terminate it. This becomes a problem with the nano_wallet when closed by the user as the child process continues to exist. I could send an HTTP request to the RPC server from the wallet, but it currently doesn't know about the rpc_config.json file so would require a lot more changes. I've now set conditional compilation depending on the boost version, so it uses the boost interprocess stuff if not using 1.69 otherwise if rpc_enable is true it errors out with a warning which states to start the RPC process manually. I've not done this for nano_node which is either forcefully closed from the command line or closed using stop () RPC command, which seems to clean up both processes properly, but it's possible it could have issues as well in some circumstances.

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

(edit: this is now implemented, thanks @wezrule!)

Putting this up for debate:

Given how hard cross-platform process mgmt can be to get right, I wonder if (at least for a version or two) we should offer a fallback to the old in-process RPC 🤔 Is that hard to get going architecturally? If the core of the externalized RPC was a shared library, the node could link it but only enable if config says "inprocess" or something like that. Alternatively, keep the original RPC connection code (default disabled) while keeping the external RPC as-is (probably the least amount of headache, but probably some things to think about config-wise)

Once external RPC and IPC is battle tested, we can remove the inprocess stuff. Of course the inprocess variation would go straight to rpc handlers, not via IPC.

Another reason for an inprocess option is that we end up using more memory because two processes now holds data during the requests. This might be a problem on small VPS's when the RPC process runs on the same machine as the node (there have been some reports on Discord about high memory usage, and there's #1750)

Show resolved Hide resolved nano/nano_wallet/entry.cpp Outdated

wezrule added some commits Apr 6, 2019

@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 6, 2019

Putting this up for debate:

Given how hard cross-platform process mgmt can be to get right, I wonder if (at least for a version or two) we should offer a fallback to the old in-process RPC 🤔 Is that hard to get going architecturally? If the core of the externalized RPC was a shared library, the node could link it but only enable if config says "inprocess" or something like that. Alternatively, keep the original RPC connection code (default disabled) while keeping the external RPC as-is (probably the least amount of headache, but probably some things to think about config-wise)

Once external RPC and IPC is battle tested, we can remove the inprocess stuff. Of course the inprocess variation would go straight to rpc handlers, not via IPC.

Another reason for an inprocess option is that we end up using more memory because two processes now holds data during the requests. This might be a problem on small VPS's when the RPC process runs on the same machine as the node (there have been some reports on Discord about high memory usage, and there's #1750)

I have added an RPC in_process option now. @SergiySW commented in another PR that filenames shouldn't have underscores _ so I have changed all RPC related files to remove them. I no longer update the daemon config when migrating the RPC, as it doesn't make sense to put the options in the IPC anymore, I have stripped down the old RPC config and reused that instead. It does mean that the IPC port is not migrated to the new rpc_config.json file, but I don't think this is much of an issue as the same default port will be used and we are defaulting to in-process now so users should have no issues unless they explicitly want to use the out of process RPC which is an active choice (it saves a lot of complexity for little gain imo). I've appended some information to the end of this PR as well.

Show resolved Hide resolved nano/node/noderpcconfig.cpp Outdated
Show resolved Hide resolved nano/node/noderpcconfig.cpp Outdated
Show resolved Hide resolved nano/nano_node/entry.cpp Outdated
Show resolved Hide resolved nano/nano_rpc/entry.cpp Outdated
Show resolved Hide resolved nano/lib/rpcconfig.cpp Outdated
@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Apr 7, 2019

Both in-process (currently default which I think is correct) and out-of-process is working great on macOS. Would be nice to ship with boost 1.70 so we could get the process mgmt fix for macOS as well (must start manually now), but not sure if it'll be released in time (edit: it's released)

@wezrule wezrule referenced this pull request Apr 11, 2019

Merged

Delete unused rpc.cpp #1897

Show resolved Hide resolved nano/node/node_rpc_config.cpp Outdated
Show resolved Hide resolved nano/nano_node/entry.cpp Outdated

@wezrule wezrule requested a review from SergiySW Apr 17, 2019

@wezrule wezrule merged commit c3046c1 into nanocurrency:master Apr 21, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@wezrule wezrule deleted the wezrule:rpc_in_new_process branch Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.