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

Release limiters when response is returned #1255

Closed

Conversation

johnhferland
Copy link

fixes #1192

Before this PR

Concurrency permits are only released when the response body is closed.

After this PR

==COMMIT_MSG==
Concurrency permits are now released when the response is received.
==COMMIT_MSG==

Possible downsides?

  • It's now technically possible to have more concurrent connections than the maximum number of permits
  • This increases the rate at which limits will increase, which may increase load on servers from clients. I expected this will be particularly noticeable for reading larger objects.

@johnhferland johnhferland requested a review from a team as a code owner September 22, 2019 18:51
@changelog-app
Copy link

changelog-app bot commented Sep 22, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Concurrency permits are now released when the response is received.

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -52,7 +46,7 @@
* 429 and 503 response codes are used for backpressure, whilst 200 -> 399 request codes are used for determining
* new limits and all other codes are not factored in to timings.
* <p>
* Concurrency permits are only released when the response body is closed.
* Concurrency permits are released when the response is received.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was discussed in the past and in general would be kinda against the principle of concurrency limiting: the fact that the body is being streamed usually means that the worker thread might have been released, but streaming the body still usually consumes resources (especially if it's just a blocking read that goes from some external resource and writes to the response body).

I think the reason why you are not observing any 429 in general in that situation is that the internal product that I expect you're referring too has request/sec rate limiting implemented, not concurrency limiting. I suspect releasing more of requests here would eventually lead to 503s from the product.

@stale
Copy link

stale bot commented Oct 10, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Oct 10, 2019
@iamdanfox
Copy link
Contributor

On your original issue (this is artifacts right?) doesn't c-j-r's limit still increase, so this should only affect a cold startup?

@stale stale bot removed the stale label Oct 10, 2019
@stale
Copy link

stale bot commented Oct 24, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Oct 24, 2019
@stale stale bot closed this Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CJR limits block downloading blobs
4 participants