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

[TRIVIAL] Remove incorrect assert #4743

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Oct 4, 2023

High Level Overview of Change

An assert in PathRequest.cpp at line 541 can fire falsely in some circumstances:

The assert is saying that the only reason pathFinder would be null is if the request was aborted (connection dropped, etc.). That's what continueCallback() checks. But that is very clearly not true if you look at getPathFinder, which calls findPaths, which can return false for all kinds of reasons.

This PR removes that assert.

Type of Change

  • [X ] Bug fix (non-breaking change which fixes an issue)

Resolved #4744

* The assert is saying that the only reason pathFinder would be null is
  if the request was aborted (connection dropped, etc.). That's what
  continueCallback() checks. But that is very clearly not true if you
  look at getPathFinder, which calls findPaths, which can return false
  for all kinds of reasons.
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍

@intelliot intelliot added this to the 2.0 milestone Oct 5, 2023
@intelliot
Copy link
Collaborator

This PR has sufficient approvals. @ximinez , feel free to apply the Passed label when this is ready to merge.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 10, 2023
* upstream/develop:
  fix(CI): update workflow for uploading binaries to artifactory (4746)
@intelliot intelliot merged commit 50cc1cf into XRPLF:develop Oct 12, 2023
16 checks passed
@ximinez ximinez deleted the fix-callback-assert branch October 12, 2023 22:05
Bronek pushed a commit to Bronek/rippled that referenced this pull request Oct 17, 2023
The assert is saying that the only reason `pathFinder` would be null is
if the request was aborted (connection dropped, etc.). That's what
`continueCallback()` checks. But that is very clearly not true if you
look at `getPathFinder`, which calls `findPaths`, which can return false
for many reasons.

Fix XRPLF#4744
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
The assert is saying that the only reason `pathFinder` would be null is
if the request was aborted (connection dropped, etc.). That's what
`continueCallback()` checks. But that is very clearly not true if you
look at `getPathFinder`, which calls `findPaths`, which can return false
for many reasons.

Fix XRPLF#4744
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
Archived in project
4 participants