diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 8e47402bca..26059e8cde 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2800,6 +2800,31 @@ only the default internal rules will be used. + Default is true, to execute project specific rules. +[[rules.reductionLimit]]rules.reductionLimit:: ++ +Maximum number of Prolog reductions that can be performed when +evaluating rules for a single change. Each function call made +in user rule code, internal Gerrit Prolog code, or the Prolog +interpreter counts against this limit. ++ +Sites using very complex rules that need many reductions should +compile Prolog to Java bytecode with link:pgm-rulec.html[rulec]. +This eliminates the dynamic Prolog interpreter from charging its +own reductions against the limit, enabling more logic to execute +within the same bounds. ++ +A reductionLimit of 0 is nearly infinite, implemented by setting +the internal limit to 2^31-1. ++ +Default is 100,000 reductions (about 14 ms on Intel Core i7 CPU). + +[[rules.compileReductionLimit]]rules.compileReductionLimit:: ++ +Maximum number of Prolog reductions that can be performed when +compiling source code to internal Prolog machine code. ++ +Default is 10x reductionLimit (1,000,000). + [[execution]] === Section execution diff --git a/gerrit-server/BUCK b/gerrit-server/BUCK index da09a7fb8d..7202dc3639 100644 --- a/gerrit-server/BUCK +++ b/gerrit-server/BUCK @@ -149,8 +149,10 @@ java_test( '//gerrit-common:server', '//gerrit-reviewdb:server', '//gerrit-server/src/main/prolog:common', + '//lib:guava', '//lib:gwtorm', '//lib:junit', + '//lib:truth', '//lib/jgit:jgit', '//lib/guice:guice', '//lib/prolog:prolog-cafe', diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java b/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java index 029a5d7dfb..2afdffc7ef 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java @@ -16,6 +16,7 @@ package com.google.gerrit.rules; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -26,9 +27,11 @@ import com.google.inject.Singleton; import com.google.inject.assistedinject.Assisted; import com.googlecode.prolog_cafe.lang.BufferingPrologControl; +import com.googlecode.prolog_cafe.lang.Predicate; import com.googlecode.prolog_cafe.lang.Prolog; import com.googlecode.prolog_cafe.lang.PrologMachineCopy; +import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -65,6 +68,8 @@ public class PrologEnvironment extends BufferingPrologControl { private final Args args; private final Map, Object> storedValues; + private int reductionLimit; + private int reductionsRemaining; private List cleanup; @Inject @@ -74,6 +79,8 @@ public class PrologEnvironment extends BufferingPrologControl { setEnabled(EnumSet.allOf(Prolog.Feature.class), false); args = a; storedValues = new HashMap<>(); + reductionLimit = a.reductionLimit; + reductionsRemaining = reductionLimit; cleanup = new LinkedList<>(); } @@ -81,6 +88,28 @@ public class PrologEnvironment extends BufferingPrologControl { return args; } + @Override + public boolean isEngineStopped() { + if (super.isEngineStopped()) { + return true; + } else if (--reductionsRemaining <= 0) { + throw new ReductionLimitException(reductionLimit); + } + return false; + } + + @Override + public void setPredicate(Predicate goal) { + super.setPredicate(goal); + reductionLimit = args.reductionLimit(goal); + reductionsRemaining = reductionLimit; + } + + /** @return number of reductions during execution. */ + public int getReductions() { + return reductionLimit - reductionsRemaining; + } + /** * Lookup a stored value in the interpreter's hash manager. * @@ -154,6 +183,8 @@ public class PrologEnvironment extends BufferingPrologControl { private final PatchSetInfoFactory patchSetInfoFactory; private final IdentifiedUser.GenericFactory userFactory; private final Provider anonymousUser; + private final int reductionLimit; + private final int compileLimit; @Inject Args(ProjectCache projectCache, @@ -161,13 +192,29 @@ public class PrologEnvironment extends BufferingPrologControl { PatchListCache patchListCache, PatchSetInfoFactory patchSetInfoFactory, IdentifiedUser.GenericFactory userFactory, - Provider anonymousUser) { + Provider anonymousUser, + @GerritServerConfig Config config) { this.projectCache = projectCache; this.repositoryManager = repositoryManager; this.patchListCache = patchListCache; this.patchSetInfoFactory = patchSetInfoFactory; this.userFactory = userFactory; this.anonymousUser = anonymousUser; + + int limit = config.getInt("rules", null, "reductionLimit", 100000); + reductionLimit = limit <= 0 ? Integer.MAX_VALUE : limit; + + limit = config.getInt("rules", null, "compileReductionLimit", + (int) Math.min(10L * limit, Integer.MAX_VALUE)); + compileLimit = limit <= 0 ? Integer.MAX_VALUE : limit; + } + + private int reductionLimit(Predicate goal) { + if ("com.googlecode.prolog_cafe.builtin.PRED_consult_stream_2" + .equals(goal.getClass().getName())) { + return compileLimit; + } + return reductionLimit; } public ProjectCache getProjectCache() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/ReductionLimitException.java b/gerrit-server/src/main/java/com/google/gerrit/rules/ReductionLimitException.java new file mode 100644 index 0000000000..2c272408c3 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/rules/ReductionLimitException.java @@ -0,0 +1,24 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.rules; + +/** Thrown by {@link PrologEnvironment} if a script runs too long. */ +public class ReductionLimitException extends RuntimeException { + private static final long serialVersionUID = 1L; + + ReductionLimitException(int limit) { + super(String.format("exceeded reduction limit of %d", limit)); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java index 8d798075ec..c17136a79d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java @@ -91,6 +91,9 @@ public class TestSubmitRule implements RestModifyView { for (SubmitRecord r : records) { out.add(new Record(r, accounts)); } + if (!out.isEmpty()) { + out.get(0).prologReductionCount = evaluator.getReductionsConsumed(); + } accounts.fill(); return out; } @@ -103,6 +106,7 @@ public class TestSubmitRule implements RestModifyView { Map need; Map may; Map impossible; + Integer prologReductionCount; Record(SubmitRecord r, AccountLoader accounts) { this.status = r.status; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index 54a95f160f..deef4ea313 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.rules.PrologEnvironment; +import com.google.gerrit.rules.ReductionLimitException; import com.google.gerrit.rules.StoredValues; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.query.change.ChangeData; @@ -36,7 +37,6 @@ import com.googlecode.prolog_cafe.compiler.CompileException; import com.googlecode.prolog_cafe.lang.IntegerTerm; import com.googlecode.prolog_cafe.lang.ListTerm; import com.googlecode.prolog_cafe.lang.Prolog; -import com.googlecode.prolog_cafe.lang.PrologException; import com.googlecode.prolog_cafe.lang.StructureTerm; import com.googlecode.prolog_cafe.lang.SymbolTerm; import com.googlecode.prolog_cafe.lang.Term; @@ -107,6 +107,7 @@ public class SubmitRuleEvaluator { private boolean skipFilters; private String rule; private boolean logErrors = true; + private int reductionsConsumed; private Term submitRule; @@ -183,6 +184,11 @@ public class SubmitRuleEvaluator { return this; } + /** @return Prolog reductions consumed during evaluation. */ + public int getReductionsConsumed() { + return reductionsConsumed; + } + /** * Evaluate the submit rules. * @@ -457,10 +463,16 @@ public class SubmitRuleEvaluator { new VariableTerm())) { results.add(template[1]); } + } catch (ReductionLimitException err) { + throw new RuleEvalException(String.format( + "%s on change %d of %s", + err.getMessage(), cd.getId().get(), getProjectName())); } catch (RuntimeException err) { - throw new RuleEvalException("Exception calling " + sr - + " on change " + cd.getId() + " of " + getProjectName(), - err); + throw new RuleEvalException(String.format( + "Exception calling %s on change %d of %s", + sr, cd.getId().get(), getProjectName()), err); + } finally { + reductionsConsumed = env.getReductions(); } Term resultsTerm = toListTerm(results); @@ -539,14 +551,16 @@ public class SubmitRuleEvaluator { parentEnv.once("gerrit", filterRuleWrapperName, filterRule, results, new VariableTerm()); results = template[2]; - } catch (PrologException err) { - throw new RuleEvalException("Exception calling " + filterRule - + " on change " + cd.getId() + " of " - + parentState.getProject().getName(), err); + } catch (ReductionLimitException err) { + throw new RuleEvalException(String.format( + "%s on change %d of %s", + err.getMessage(), cd.getId().get(), parentState.getProject().getName())); } catch (RuntimeException err) { - throw new RuleEvalException("Exception calling " + filterRule - + " on change " + cd.getId() + " of " - + parentState.getProject().getName(), err); + throw new RuleEvalException(String.format( + "Exception calling %s on change %d of %s", + filterRule, cd.getId().get(), parentState.getProject().getName()), err); + } finally { + reductionsConsumed += env.getReductions(); } childEnv = parentEnv; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java index 8e1ab8fd10..d6318b7da5 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java @@ -18,6 +18,8 @@ import static com.google.gerrit.common.data.Permission.LABEL; import static com.google.gerrit.server.project.Util.allow; import static com.google.gerrit.server.project.Util.category; import static com.google.gerrit.server.project.Util.value; +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.LabelType; @@ -31,9 +33,18 @@ import com.google.gerrit.server.project.Util; import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.inject.AbstractModule; +import com.googlecode.prolog_cafe.compiler.CompileException; +import com.googlecode.prolog_cafe.lang.JavaObjectTerm; +import com.googlecode.prolog_cafe.lang.Prolog; +import com.googlecode.prolog_cafe.lang.StructureTerm; +import com.googlecode.prolog_cafe.lang.SymbolTerm; + +import org.eclipse.jgit.lib.Config; import org.junit.Before; import org.junit.Test; +import java.io.PushbackReader; +import java.io.StringReader; import java.util.Arrays; public class GerritCommonTest extends PrologTestCase { @@ -56,6 +67,9 @@ public class GerritCommonTest extends PrologTestCase { load("gerrit", "gerrit_common_test.pl", new AbstractModule() { @Override protected void configure() { + Config cfg = new Config(); + cfg.setInt("rules", null, "reductionLimit", 1300); + cfg.setInt("rules", null, "compileReductionLimit", (int) 1e6); bind(PrologEnvironment.Args.class).toInstance( new PrologEnvironment.Args( null, @@ -63,7 +77,8 @@ public class GerritCommonTest extends PrologTestCase { null, null, null, - null)); + null, + cfg)); } }); @@ -92,4 +107,30 @@ public class GerritCommonTest extends PrologTestCase { public void testGerritCommon() { runPrologBasedTests(); } + + @Test + public void testReductionLimit() throws CompileException { + PrologEnvironment env = envFactory.create(machine); + setUpEnvironment(env); + env.setEnabled(Prolog.Feature.IO, true); + + String script = "loopy :- b(5).\n" + + "b(N) :- N > 0, !, S = N - 1, b(S).\n" + + "b(_) :- true.\n"; + + SymbolTerm nameTerm = SymbolTerm.create("testReductionLimit"); + JavaObjectTerm inTerm = new JavaObjectTerm( + new PushbackReader(new StringReader(script), Prolog.PUSHBACK_SIZE)); + if (!env.execute(Prolog.BUILTIN, "consult_stream", nameTerm, inTerm)) { + throw new CompileException("Cannot consult " + nameTerm); + } + + try { + env.once(Prolog.BUILTIN, "call", new StructureTerm(":", + SymbolTerm.create("user"), SymbolTerm.create("loopy"))); + fail("long running loop did not abort with ReductionLimitException"); + } catch (ReductionLimitException e) { + assertThat(e.getMessage()).isEqualTo("exceeded reduction limit of 1300"); + } + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/rules/PrologTestCase.java b/gerrit-server/src/test/java/com/google/gerrit/rules/PrologTestCase.java index 0f00afbe5e..aaab1739f9 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/rules/PrologTestCase.java +++ b/gerrit-server/src/test/java/com/google/gerrit/rules/PrologTestCase.java @@ -52,8 +52,8 @@ public abstract class PrologTestCase { private boolean hasSetup; private boolean hasTeardown; private List tests; - private PrologMachineCopy machine; - private PrologEnvironment.Factory envFactory; + protected PrologMachineCopy machine; + protected PrologEnvironment.Factory envFactory; protected void load(String pkg, String prologResource, Module... modules) throws CompileException, IOException {