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

Remove always-false check of retryCurrentRoute() and unreachable code. #5762

Merged
merged 1 commit into from Feb 6, 2020
Merged

Remove always-false check of retryCurrentRoute() and unreachable code. #5762

merged 1 commit into from Feb 6, 2020

Conversation

rengwuxian
Copy link
Contributor

@rengwuxian rengwuxian commented Jan 31, 2020

This retryCurrentRoute() check will always return false, because transmitter.connection is null here.

Reason:
Just a few lines above, result is set to transmitter.connection, with the condition transmitter.connection != null. And as result == null is the entry condition of this lower block, transmitter.connection should definitely be null, which makes retryCurrentRoute() always return false.

// findConnection()
// wrapped in a synchronized block

if (transmitter.connection != null) {
    // We had an already-allocated connection and it's good.
    result = transmitter.connection
    releasedConnection = null
}

if (result == null) {
  // Attempt to get a connection from the pool.
  if (connectionPool.transmitterAcquirePooledConnection(address, transmitter, null, false)) {
    foundPooledConnection = true
    result = transmitter.connection
  } else if (nextRouteToTry != null) {
    selectedRoute = nextRouteToTry
    nextRouteToTry = null
  } else if (retryCurrentRoute()) {
    selectedRoute = transmitter.connection!!.route()
  }
}


...


private fun retryCurrentRoute(): Boolean {
  return transmitter.connection != null &&
      transmitter.connection!!.routeFailureCount == 0 &&
      transmitter.connection!!.route().address.url.canReuseConnectionFor(address.url)
}

@rengwuxian
Copy link
Contributor Author

@swankjesse Can you take a look at this? I know you're preparing for the next release of OkHttp, but I'm not just "jump into the code and find a stain then hurry to clean it without thinking". I've read the total workflow of OkHttp 4.3.1, and I'm 99% sure these 2 lines are safe to delete.

  1. nextRouteToTry looks like (I'm not the author of it, so I'd rather use "looks like" than "is") a marker for a later round of connection creation, just as an [IP address + port + Proxy] combination, so that you don't have to call RouteSelection.next();
  2. But I don't see how the next 2 lines (which is deleted by me) works. they look like another quick pick, but the line inside the curly braces seems not able to be reached.

Or have I missed something? As I said, I'm 99% sure, not 100% 😂 .

@rengwuxian
Copy link
Contributor Author

Resolved conflicts with the latest code.

@swankjesse
Copy link
Member

Code coverage agrees!

Screen Shot 2020-02-05 at 9 21 41 PM

@swankjesse
Copy link
Member

Love it. Agreed with your reasoning. Nice find!

@swankjesse swankjesse merged commit 0d213ba into square:master Feb 6, 2020
@rengwuxian
Copy link
Contributor Author

😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants