Merge "Replace Semaphore with AtomicInteger"
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java b/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
index 53651eb..2c24f1d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
@@ -23,7 +23,6 @@
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupMembership;
-import com.google.gerrit.server.group.GroupResource;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.restapi.group.GroupsCollection;
import com.google.inject.Inject;
@@ -57,28 +56,35 @@
if (limits.isPresent()) {
GroupMembership memberShip = user.getEffectiveGroups();
for (String groupName : limits.get().keySet()) {
- try {
- GroupResource group =
- groupsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(groupName));
- Optional<GroupDescription.Internal> maybeInternalGroup = group.asInternalGroup();
- if (!maybeInternalGroup.isPresent()) {
- log.debug("Ignoring limits for non-internal group ''{}'' in quota.config", groupName);
- } else if (memberShip.contains(maybeInternalGroup.get().getGroupUUID())) {
- RateLimit groupLimit = limits.get().get(groupName);
- if (groupLimit != null) {
- return Optional.of(groupLimit);
- }
- }
- } catch (ResourceNotFoundException e) {
- log.debug("Ignoring limits for unknown group ''{}'' in quota.config", groupName);
- } catch (AuthException e) {
- log.debug("Ignoring limits for non-visible group ''{}'' in quota.config", groupName);
+ RateLimit groupLimit = limits.get().get(groupName);
+ if (groupLimit != null && isMatching(memberShip, groupName)) {
+ return Optional.of(groupLimit);
}
}
}
return Optional.empty();
}
+ public boolean isMatching(GroupMembership membership, String groupName) {
+ try {
+ Optional<GroupDescription.Internal> maybeInternalGroup =
+ groupsCollection
+ .parse(TopLevelResource.INSTANCE, IdString.fromDecoded(groupName))
+ .asInternalGroup();
+ if (!maybeInternalGroup.isPresent()) {
+ log.debug("Ignoring limits for non-internal group ''{}'' in quota.config", groupName);
+ } else if (membership.contains(maybeInternalGroup.get().getGroupUUID())) {
+ return true;
+ }
+ } catch (ResourceNotFoundException e) {
+ log.debug("Ignoring limits for unknown group ''{}'' in quota.config", groupName);
+ } catch (AuthException e) {
+ log.debug("Ignoring limits for non-visible group ''{}'' in quota.config", groupName);
+ }
+
+ return false;
+ }
+
/**
* @param type type of rate limit
* @return global rate limit
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java b/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java
index 28c0a60..8f19b37 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java
@@ -51,6 +51,7 @@
@Override
protected void configure() {
DynamicSet.bind(binder(), AllRequestFilter.class).to(RestApiRateLimiter.class);
+ DynamicSet.bind(binder(), AllRequestFilter.class).to(MaxConnectionsLimiter.class);
bindConstant()
.annotatedWith(Names.named(RateMsgHelper.RESTAPI_CONFIGURABLE_MSG_ANNOTATION))
.to(restapiLimitExceededMsg);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/MaxConnectionsLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/quota/MaxConnectionsLimiter.java
new file mode 100644
index 0000000..f2b18cd
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/quota/MaxConnectionsLimiter.java
@@ -0,0 +1,145 @@
+package com.googlesource.gerrit.plugins.quota;
+
+import com.google.gerrit.httpd.AllRequestFilter;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import org.eclipse.jgit.lib.Config;
+
+import java.io.IOException;
+import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+@Singleton
+public class MaxConnectionsLimiter extends AllRequestFilter {
+ record Limit(String group, Integer restApiLimit) {}
+
+ private static final String GLOBAL_KEY = "global";
+ private static final String CONFIG_KEY = "maxConnectionsPerUserForTask";
+ private static final String CONFIG_REST_API = "rest-api";
+ private static final Pattern REST_API_REGEX = Pattern.compile("(\\d+)\\s+" + CONFIG_REST_API);
+
+ private static final Pattern REST_API_PATH_PATTERN =
+ Pattern.compile(
+ "^/(?:a/)?(access|accounts|changes|config|groups|plugins|projects|tools)/.*$");
+
+ private final Map<String, Integer> connectionsByUser = new ConcurrentHashMap<>();
+ private final Provider<CurrentUser> userProvider;
+ private final AccountLimitsFinder accountLimitsFinder;
+ private final List<Limit> limits = new ArrayList<>(2);
+ private Limit globalLimit;
+
+ @Inject
+ public MaxConnectionsLimiter(
+ AccountLimitsFinder accountLimitsFinder, Provider<CurrentUser> userProvider) {
+ this.accountLimitsFinder = accountLimitsFinder;
+ this.userProvider = userProvider;
+ }
+
+ @Inject
+ void init(ProjectCache projectCache) {
+ Config cfg = projectCache.getAllProjects().getConfig("quota.config").get();
+ for (String group : cfg.getSubsections(AccountLimitsConfig.GROUP_SECTION)) {
+ String val = cfg.getString(AccountLimitsConfig.GROUP_SECTION, group, CONFIG_KEY);
+ Matcher matcher = REST_API_REGEX.matcher(val);
+ if (matcher.matches()) {
+ limits.add(new Limit(group, Integer.parseInt(matcher.group(1))));
+ }
+ }
+
+ String globalConfig = cfg.getString(GLOBAL_KEY, null, CONFIG_KEY);
+ if (globalConfig != null) {
+ Matcher matcher = REST_API_REGEX.matcher(globalConfig);
+ if (matcher.matches()) {
+ globalLimit = new Limit(GLOBAL_KEY, Integer.parseInt(matcher.group(1)));
+ }
+ }
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+ throws IOException, ServletException {
+ if (isRestApiRequest(request)) {
+ CurrentUser currentUser = userProvider.get();
+
+ if (currentUser.isIdentifiedUser()) {
+ String userId = currentUser.getAccountId().toString();
+ Optional<Integer> limit = getEffectiveRestApiLimit(currentUser.asIdentifiedUser());
+
+ if (limit.isPresent()) {
+ if (!canPermitCall(userId, limit.get())) {
+ ((HttpServletResponse) response)
+ .sendError(429, "Too Many Requests: rate limited by " + CONFIG_KEY);
+ return;
+ }
+ try {
+ chain.doFilter(request, response);
+ } finally {
+ markCallComplete(userId);
+ }
+ return;
+ }
+ }
+ }
+
+ chain.doFilter(request, response);
+ }
+
+ private boolean isRestApiRequest(ServletRequest req) {
+ return req instanceof HttpServletRequest
+ && REST_API_PATH_PATTERN.matcher(((HttpServletRequest) req).getServletPath()).matches();
+ }
+
+ private Optional<Integer> getEffectiveRestApiLimit(IdentifiedUser user) {
+ List<Integer> result = new ArrayList<>();
+
+ for (Limit limit : limits) {
+ if (accountLimitsFinder.isMatching(user.getEffectiveGroups(), limit.group())) {
+ result.add(limit.restApiLimit());
+ break; // only consider the first matching group
+ }
+ }
+
+ if (globalLimit != null) {
+ result.add(globalLimit.restApiLimit());
+ }
+
+ return result.stream().filter(l -> l > 0).min(Integer::compareTo);
+ }
+
+ private boolean canPermitCall(String userId, int limit) {
+ AtomicBoolean permitted = new AtomicBoolean(false);
+ connectionsByUser.compute(
+ userId,
+ (u, c) -> {
+ int current = (c == null) ? 0 : c;
+ if (current < limit) {
+ permitted.setPlain(true);
+ return current + 1;
+ }
+ return current;
+ });
+ return permitted.getPlain();
+ }
+
+ private void markCallComplete(String userId) {
+ connectionsByUser.computeIfPresent(
+ userId,
+ (u, c) -> {
+ c--;
+ return c <= 0 ? null : c;
+ });
+ }
+}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 76a0491..6ae88af 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -255,6 +255,19 @@
`Exceeded rate limit of ${rateLimit} REST API requests/hour (or idle `
`time used up in bursts of max ${burstsLimit} requests)` .
+<a id="maxConnectionsPerUserForTask" />
+`maxConnectionsPerUserForTask`
+: Even though we have ratelimiting over a window of period, costly restapis
+run concurrently by users can still bring down the server. Using the
+below config we could limit the concurrent calls. This helps in limiting the
+costly calls that can occupy threads for long periods and prevent other shorter
+operations by other users from being processed.
+
+```
+ [group "Registered Users"]
+ maxConnectionsPerUserForTask = 20 rest-api
+```
+
Task Quota
-----------
