Fix nullptr resolving, no db config

[oleks-rip]

High Level Overview of Change

Fix resolving nullptr

Context of Change

If config disable sql db usage (like validator)

[ledger_tx_tables]
use_tx_tables = 0

then pointer to DB engine is null, but still resolved during startup. I suppose that there is no crash for now because of c++ optimizer which drop the code with resolving pointer. Reproduced 100% in debug mode.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

[pratikmankawde]

Sorry, I don't understand how this is solving the issue of potentially null txnDB. Instead of resolving this ptr in SQLiteDatabaseImp::saveValidatedLedger, we are now resolving it in Node::saveValidatedLedger.

Shouldn't we rather add some checks on this pointer before calling Node::saveValidatedLedger? Or maybe assert the validity of the pointer in SQLiteDatabaseImp::saveValidatedLedger (assuming ripple's version of assert will throw in release mode.)

[oleks-rip]

Please check line 255, if (app.config().useTxTables()) is a condition for db present

[pratikmankawde]

Yeah, but does that also guarantee that the pointer will be valid? I believe the check only confirms that there's an entry in the config. We should ideally confirm if the database connection was successful.

[oleks-rip]

It is checked in DB connection creation, under the same condition - see node.cpp:64

[vvysokikh1]

while this is correct currently - it might not be always so. This is very smelly solution. Also a reference to a unique ptr is another bad pattern.

In case you want to resolve it more a less normal, please either pass a raw pointer, or an optional<reference_wrapper...>. Check them at the point where you dereference them

[a1q123456]

I think it'd be nice to have a helper class borrowed_ptr<T> which doesn't do anything except that it's borrowing the pointer and the ownership is managed by something else like a shared_ptr or a unique_ptr, but it sounds like a separate task.
But IMHO, I don't think raw pointers are bad at all and we shouldn't be scared of them.

[oleks-rip]

I don't understand the problem of passing unique_ptr ref. It is the same thing as passing vector / shared_ptr or any other container by ref. "Borrowing" pointer, direct pointer - why to use them, when you don't know their lifetime and even check that they are not null doesn't guarantee you anything? There is already pointer without ownership in std - weak_ptr, and there is a good reason why you can't use it directly.

[a1q123456]

We shouldn’t expose the type of the pointer to the caller. i.e. The caller shouldn’t have to have a unique_ptr or a shared_ptr because the function only wants to take an optional value, it does not care who manages the resources and it’s leaking abstraction.

[oleks-rip]

Unique ptr is not some special container. We don't pass pointers instead of array/vector reference to maintain more abstraction. Using pointers without real need is a bad pattern.

[a1q123456]

I don’t think a raw pointer is a bad pattern

[vvysokikh1]

while this is correct currently - it might not be always so. This is very smelly solution. Also a reference to a unique ptr is another bad pattern.

In case you want to resolve it more a less normal, please either pass a raw pointer, or an optional<reference_wrapper...>. Check them at the point where you dereference them

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.6%. Comparing base (58e0319) to head (ccd8042).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6029   +/-   ##
=======================================
  Coverage     78.6%   78.6%           
=======================================
  Files          818     818           
  Lines        68981   68981           
  Branches      8248    8238   -10     
=======================================
+ Hits         54191   54205   +14     
+ Misses       14790   14776   -14     

Files with missing lines	Coverage Δ
src/xrpld/app/rdb/backend/detail/Node.cpp	58.8% <100.0%> (+0.1%)	⬆️
...rc/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp	58.4% <100.0%> (-0.1%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[a1q123456]

I'm going to approve it as there's nothing more we can do, and it solves the problem.

[oleks-rip]

@vvysokikh1 please close your change request

myself

[vvysokikh1]

@vvysokikh1 please close your change request

sorry for the delay, I thought I discarded my review back when I agreed with your point

[pratikmankawde]

I see that other reviewers are fine with this change, so I am not going to block this. But I don't like the idea that, since one part of the code uses (not establishes) correlation between two entities, we should assume that such correlation will persist for the lifetime of these entities (in this case, the config setting and the database connection object). Any intermediate call can reset the database connection object. The database connection creation process is throwing exceptions on failure. A caller might catch those and still proceed with rest of the code. Then some branch of the code can still decide to call saveValidatedLedger (because of developer mistake and their assumption that since the code reaches here, the connection object should be fine.).

In this PR, I am not suggesting to traverse the whole repo and update code which doesn't check validity of txnDB when we use it(although we should, in a dedicated PR). But for any new code we introduce, we should fix the wrong pattern being followed and definitely should not keep following it. I agree, we don't need to test txnDB every time we use it, we can assume it is valid after we check it at the beginning of a code flow. But checking a config variable and assuming its corresponding object would be valid, is, I would say, equivalent to introducing a bug. Specially when the cost of checking txnDB is smaller than evaluating app.config().useTxTables() (assuming there's no chained-inlining and compiler optimisation being done on this statement. Since compilers usually ignore chained inlining, even if explicitly suggested.).

[oleks-rip]

But checking a config variable and assuming its corresponding object would be valid, is, I would say, equivalent to introducing a bug. Specially when the cost of checking txnDB is smaller than evaluating app.config().useTxTables() (assuming there's no chained-inlining and compiler optimisation being done on this statement. Since compilers usually ignore chained inlining, even if explicitly suggested.).

I did't dig that deep, may be there is situation when txnDB can be valid DB connection, but config still doesn't allow to use txnTables, and that logic connected to config settings, not just to availability of the DB. This PR fix concrete problem with the fewest possible changes and try to don't change any other logic.

[pratikmankawde]

I did't dig that deep, may be there is situation when txnDB can be valid DB connection, but config still doesn't allow to use txnTables, and that logic connected to config settings, not just to availability of the DB. This PR fix concrete problem with the fewest possible changes and try to don't change any other logic.

Even then, replacing

if (app.config().useTxTables())
{
    auto db = txnDB->checkoutDb();

with

if (app.config().useTxTables() && txnDB)
{
    auto db = txnDB->checkoutDb();

would be much better.

[oleks-rip]

Added && txnDB

[pratikmankawde]

Added && txnDB

Thanks

[oleks-rip]

Change it to throw an error

[pratikmankawde]

Now a runtime error is a change in behaviour of the program. I am not sure if this should be the expected behaviour, so I will let others confirm this.

[oleks-rip]

Disagree. Adding check if (app.config().useTxTables() && txnDB) will silently continue the code which is change of the current behavior. That's why I change it to throwing error, at least it will reproduce debug behavior, cause release behavior is undefined in case of nullptr.

[pratikmankawde]

Since the old code was buggy, we don't need to consider it's behaviour anymore. You now have the power to decide the new behaviour of the new code. And throwing a runtime error is such change. That's why it is important to consider if we want to crash the app and let it be relaunched, or throw then catch and then retry creating the DB connection at this point. Or even just not save the ledger state here and spit an error message(discard the transaction? Initiate a resync?). And such decisions need to be agreed upon and well documented. Sometimes such trivial looking changes could have an unexpected impact on the product.

[oleks-rip]

This additional check (if (!txnDB)) is pure placebo, it will never be reached with the current code (because of exception throwing while establishing connection to the DB). So the behavior is exactly the same as before. If someone will change behavior and will allow empty DB pointer, they will get an exception and will know that they need to make changes to this part of the code too.
In case of if (app.config().useTxTables() && txnDB) check and possible future code change - the code will silently proceed skipping initialization, and developer will miss that they introduce silent bug.
I don't quite understand what is your point ?

[pratikmankawde]

My point is, just deciding to throw a runtime error is the easy way out but may not be the right way forward. Hence it is important to understand all potential routes which could lead to here(this part of the code) and then decide on potential way forward.

For now, this might be good enough. But we should definitely have a more detailed discussion about such patterns and fixing rest of the code later.

[bthomee]

@oleks-rip out of curiosity, is it ever possible for txnDB to be valid at first and then to become invalid over time?

I'm wondering whether the code could be changed to check txnDB on startup and throw right away when app.config().useTxTables() is enabled but txnDB is invalid. Then, each time we get here we only need to check if (!txnDB) and can remove the app.config().useTxTables() check + throw.

[oleks-rip]

The reason why it is implemented here like this is to NOT to change existing logic.

Your proposal is most straightforward check that should be implemented.
But, theoretically, txnDB can be closed and set pointer to null at any time in the program for multiple reasons (errorrs, hardware failures, db driver exceptions, some multithread collisions). And we need to check all the use cases for the DB to be ensure. Such logic improvement wasn't the purpose of this patch, which is small and intended only to fix a crash.

[bthomee]

Thanks - this is very helpful.

[pratikmankawde]

Approving.