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

Cleanly report invalid [server] settings (RIPD-1562) #2305

Closed
wants to merge 3 commits into from

Conversation

bachase
Copy link
Collaborator

@bachase bachase commented Dec 18, 2017

Ensure when standing up the server at startup, we catch any exceptions related to invalid [server] settings and cleanly report them before shutting down.

@ripplelabs-jenkins
Copy link
Collaborator

ripplelabs-jenkins commented Dec 18, 2017

Jenkins Build Summary

Built from this commit

Built at 20171220 - 19:38:23

Test Results

Build Type Result Status
coverage 970 cases, 0 failed, t: 605s PASS ✅
clang.debug.unity 970 cases, 0 failed, t: 386s PASS ✅
gcc.debug.unity 970 cases, 0 failed, t: 430s PASS ✅
clang.debug.nounity 968 cases, 0 failed, t: 700s PASS ✅
gcc.debug.nounity 968 cases, 0 failed, t: 708s PASS ✅
clang.release.unity 969 cases, 0 failed, t: 466s PASS ✅
gcc.release.unity 969 cases, 0 failed, t: 509s PASS ✅

@codecov-io
Copy link

codecov-io commented Dec 18, 2017

Codecov Report

Merging #2305 into develop will increase coverage by 0.7%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #2305     +/-   ##
==========================================
+ Coverage    70.23%   70.93%   +0.7%     
==========================================
  Files          706      691     -15     
  Lines        61334    51391   -9943     
==========================================
- Hits         43077    36456   -6621     
+ Misses       18257    14935   -3322
Impacted Files Coverage Δ
src/ripple/app/main/Application.cpp 60.93% <100%> (-14.38%) ⬇️
src/ripple/server/impl/Port.cpp 75.21% <12.5%> (+4.9%) ⬆️
src/ripple/rpc/impl/ServerHandlerImp.cpp 89.96% <62.5%> (-0.09%) ⬇️
src/ripple/overlay/impl/PeerSet.cpp 33.89% <0%> (-30.51%) ⬇️
src/ripple/shamap/impl/SHAMapNodeID.cpp 63.07% <0%> (-12.31%) ⬇️
src/ripple/app/ledger/impl/InboundLedger.cpp 24.96% <0%> (-9.38%) ⬇️
src/ripple/beast/clock/chrono_util.h 82.6% <0%> (-8.7%) ⬇️
src/ripple/app/ledger/TransactionStateSF.cpp 53.84% <0%> (-8.66%) ⬇️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
... and 197 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc9e9f4...890a56f. Read the comment docs.

Copy link
Contributor

@mellery451 mellery451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. just a note that these errors are treated as warning by the rpcClient (rippled as client) and that means you get some other subsequent failure depending on the field that was missing or invalid.

}
catch (std::exception const&)
{
JLOG(m_journal.fatal()) << "Unable to setup server handler";
Copy link
Contributor

@miguelportilla miguelportilla Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to log the exception message if one exists. Even if the current code is unlikely to produce one now, it might catch a new message in the future.

Just an idea- If the exception message was logged here, throws in to_Port or populate etc.. could include the message instead of logging and throwing which seems kinda kludgy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestions. I went with printing the message if one exists to keep a limited scope of this PR.

Copy link
Contributor

@miguelportilla miguelportilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nbougalis nbougalis added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Dec 20, 2017
@@ -893,19 +893,19 @@ to_Port(ParsedPort const& parsed, std::ostream& log)

if (! parsed.ip)
{
log << "Missing 'ip' in [" << p.name << "]\n";
log << "Missing 'ip' in [" << p.name << "]";
Throw<std::exception> ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Food for thought: why not do something like:

Throwstd::runtime_exception("Missing 'ip' in [" + p.name + "]");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miguelportilla suggested a similar change.

I had gone down that route (along with eliminating the exceptions altogether), but it was starting to touch more code than I was comfortable with for this targeted change. I wasn't sure if some callers wanted to distinguish logging from exception handling.

@nbougalis nbougalis mentioned this pull request Jan 10, 2018
@seelabs
Copy link
Collaborator

seelabs commented Jan 16, 2018

In 0.90.0-b3

@seelabs seelabs closed this Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants