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

Review HTTP client feature setRemoveIdleDestinations #8493

Closed
lorban opened this issue Aug 30, 2022 · 1 comment · Fixed by #8530
Closed

Review HTTP client feature setRemoveIdleDestinations #8493

lorban opened this issue Aug 30, 2022 · 1 comment · Fixed by #8530
Assignees
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement

Comments

@lorban
Copy link
Contributor

lorban commented Aug 30, 2022

Jetty version(s)
9.4.x, 10.0.x+

Description
The removeIdleDestinations feature always was racy, but with the advent of #5218 the race went from benign to problematic, because the Pool object does not give back connections when it is closed.

Also review why we use both HttpDestination.close() and HttpDestination.doStop() to manage lifecycle, and why HttpDestination is both added as lifecycle bean and in a Map within HttpClient.

@lorban lorban added Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement and removed Sponsored This issue affects a user with a commercial support agreement labels Aug 30, 2022
@sbordet
Copy link
Contributor

sbordet commented Aug 30, 2022

@lorban here's an idea for fixing this issue.

HttpClient.resolveDestination() is implemented to call a new method, HttpDestination.stale() (not just a getter because it has a side effect).

boolean HttpDestination::stale() {
  lock {
    boolean stale = _stale;
    if (!stale) {
      _staleCount = 0;
    }
    return stale;
  }
}

Then, if removeIdleDestination==true, we use a Sweeper to periodically sweep destinations:

boolean HttpDestination::sweep() {
  boolean remove = false;
  lock {
    boolean stale = _exchanges.isEmpty() && _connectionPool.isEmpty();
    if (stale)
      ++_staleCount;
    else
      _staleCount = 0;
    if (_staleCount == 4) {
      _stale = true;
      remove = true;
    }
  }
  if (remove) {
    getHttpClient().removeDestination(this);
    LifeCycle.stop(this);
  }
  return remove;
}

The sweeper's attempt to remove/stop the destination gets pushed into the future every time resolveDestination() is called, because _staleCount is reset, so the sweeper loses the race with the call to stale().
If the sweeper wins the race, then _stale==true and resolveDestination() will create a new destination object for the same origin.

HttpClient.removeDestination() needs to be implemented by calling Map.remove(key, value) to be sure to remove the stale destination and not the new one possibly created by resolveDestination().

At this point we know that the stale destination cannot be referenced for new requests, and it is not processing any request (as it has been stale for 4 sweeper cycles) therefore it can be safely stopped.

@lorban lorban self-assigned this Aug 31, 2022
lorban added a commit that referenced this issue Aug 31, 2022
lorban added a commit that referenced this issue Aug 31, 2022
lorban added a commit that referenced this issue Aug 31, 2022
lorban added a commit that referenced this issue Aug 31, 2022
lorban added a commit that referenced this issue Aug 31, 2022
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Aug 31, 2022
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Aug 31, 2022
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Sep 1, 2022
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Sep 1, 2022
lorban added a commit that referenced this issue Sep 1, 2022
lorban added a commit that referenced this issue Sep 1, 2022
Fixes #8493: RemoveIdleDestinations's race condition and improve logging.

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban closed this as completed in de13cef Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants