Replace some asserts in PeerFinder::Logic with LogicError #4562
Conversation
|
Can you explain how this is a breaking change? I think I would consider it a Refactor, if we believe the following to be accurate: BeforeIn a release build, in certain situations, the server would crash and not definitively indicate the nature of the error. AfterIn a release build, in those same certain situations, the server would crash and definitively indicate the nature of the error, including a reasonable error message. Reasoning against "Breaking change"There is no situation in which
|
|
After writing the Before / After above, I think one could even argue for "New feature", since it adds the "functionality" of a potentially better error message for certain crashes. |
|
@intelliot, that's a fair description. I very much agree that what would have been a puzzling crash before will now become a crash with an informative error message. I'm happy to call this a refactor. |
|
With a removed .conan/data folder and using this command: I'm getting a bunch of errors building boost 1.77. I believe this probably just needs to be brought up to boost 1.82, but I'm not positive. My build system has been somewhat in flux over the past week. |
|
Hi @HowardHinnant. My best guess for the problems you're experiencing is that the branch is really stale. So I just applied a merge commit. With luck that will help the situation. |
|
@scottschurr : at your convenience, can you confirm whether this PR is ready to merge? (If you have permission - I'm not sure if you do - you can put the |
scottschurr
added
the
Passed
There was a problem hiding this comment.
Suggested commit message:
refactor(peerfinder): use LogicError in PeerFinder::Logic (#4562)
It might be possible for the server code to indirect through certain
`end()` iterators. While a debug build would catch this problem with
`assert()`s, a release build would crash. If there are problems in this
area in the future, it is best to get a definitive indication of the
nature of the error regardless of whether it's a debug or release build.
To accomplish this, these `assert`s are converted into `LogicError`s
that will produce a reasonable error message when they fire.
|
Suggested commit message looks good to me. Do you want me to squash and change the commit message? Or will you do that when you merge to develop? |
|
I will do it |
It might be possible for the server code to indirect through certain `end()` iterators. While a debug build would catch this problem with `assert()`s, a release build would crash. If there are problems in this area in the future, it is best to get a definitive indication of the nature of the error regardless of whether it's a debug or release build. To accomplish this, these `assert`s are converted into `LogicError`s that will produce a reasonable error message when they fire.
High Level Overview of Change
It was discovered that it might be possible for the rippled server code to indirect through certain
end()iterators. This problem is caught byassert()s in a debug build, but a release build would simply crash.If we see problems in this area in the future, we'd like to get a definitive indication of the nature of the error regardless of whether it's a debug or release build. To accomplish this, these
asserts are converted intoLogicErrors that will produce a reasonable error message when they fire.Type of Change