Skip to content

Commit f57f672

Browse files
criemencopybara-github
criemen
authored and
copybara-github
committed
Remove DownloadManager instance from RegistryFactoryImpl.
This PR removes the DownloadManager from the registry factory implementation. As the registry created by the factory is cached by the SkyFunction mechanism, the DownloadManager instance was living too long - it is supposed to be re-created for every command instantiation to respect changes in command line options, but for the registry, it ignored those changes. Instead, the DownloadManager is set directly into the affected SkyFunctions that require access to it. This way, the per-command DownloadManager instance is correctly used. This fixes #24166. Note for reviewers: This is my first time touching code with SkyFunctions, so I don't really know what I'm doing. Closes #24212. PiperOrigin-RevId: 693644409 Change-Id: I7b16684e52673043615290d114f078ab7ab99fcf
1 parent 2fd6115 commit f57f672

File tree

10 files changed

+144
-86
lines changed

10 files changed

+144
-86
lines changed

‎src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,9 @@ public class BazelRepositoryModule extends BlazeModule {
161161
private List<String> allowedYankedVersions = ImmutableList.of();
162162
private boolean disableNativeRepoRules;
163163
private SingleExtensionEvalFunction singleExtensionEvalFunction;
164+
private ModuleFileFunction moduleFileFunction;
165+
private RepoSpecFunction repoSpecFunction;
166+
private YankedVersionsFunction yankedVersionsFunction;
164167

165168
private final VendorCommand vendorCommand = new VendorCommand(clientEnvironmentSupplier);
166169
private final RegistryFactoryImpl registryFactory =
@@ -248,14 +251,17 @@ public void workspaceInit(
248251
builtinModules = ModuleFileFunction.getBuiltinModules(directories.getEmbeddedBinariesRoot());
249252
}
250253

254+
moduleFileFunction =
255+
new ModuleFileFunction(
256+
runtime.getRuleClassProvider().getBazelStarlarkEnvironment(),
257+
directories.getWorkspace(),
258+
builtinModules);
259+
repoSpecFunction = new RepoSpecFunction();
260+
yankedVersionsFunction = new YankedVersionsFunction();
261+
251262
builder
252263
.addSkyFunction(SkyFunctions.REPOSITORY_DIRECTORY, repositoryDelegatorFunction)
253-
.addSkyFunction(
254-
SkyFunctions.MODULE_FILE,
255-
new ModuleFileFunction(
256-
runtime.getRuleClassProvider().getBazelStarlarkEnvironment(),
257-
directories.getWorkspace(),
258-
builtinModules))
264+
.addSkyFunction(SkyFunctions.MODULE_FILE, moduleFileFunction)
259265
.addSkyFunction(SkyFunctions.BAZEL_DEP_GRAPH, new BazelDepGraphFunction())
260266
.addSkyFunction(
261267
SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace()))
@@ -269,8 +275,8 @@ SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace
269275
.addSkyFunction(
270276
SkyFunctions.REGISTRY,
271277
new RegistryFunction(registryFactory, directories.getWorkspace()))
272-
.addSkyFunction(SkyFunctions.REPO_SPEC, new RepoSpecFunction())
273-
.addSkyFunction(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction())
278+
.addSkyFunction(SkyFunctions.REPO_SPEC, repoSpecFunction)
279+
.addSkyFunction(SkyFunctions.YANKED_VERSIONS, yankedVersionsFunction)
274280
.addSkyFunction(
275281
SkyFunctions.VENDOR_FILE,
276282
new VendorFileFunction(runtime.getRuleClassProvider().getBazelStarlarkEnvironment()))
@@ -305,8 +311,10 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
305311
DownloadManager downloadManager =
306312
new DownloadManager(repositoryCache, env.getDownloaderDelegate(), env.getHttpDownloader());
307313
this.starlarkRepositoryFunction.setDownloadManager(downloadManager);
314+
this.moduleFileFunction.setDownloadManager(downloadManager);
315+
this.repoSpecFunction.setDownloadManager(downloadManager);
316+
this.yankedVersionsFunction.setDownloadManager(downloadManager);
308317
this.vendorCommand.setDownloadManager(downloadManager);
309-
this.registryFactory.setDownloadManager(downloadManager);
310318

311319
clientEnvironmentSupplier.set(env.getRepoEnv());
312320
PackageOptions pkgOptions = env.getOptions().getOptions(PackageOptions.class);

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java‎

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ public enum KnownFileHashesMode {
8585
}
8686

8787
private final URI uri;
88-
private final DownloadManager downloadManager;
8988
private final Map<String, String> clientEnv;
9089
private final Gson gson;
9190
private final ImmutableMap<String, Optional<Checksum>> knownFileHashes;
@@ -99,14 +98,12 @@ public enum KnownFileHashesMode {
9998

10099
public IndexRegistry(
101100
URI uri,
102-
DownloadManager downloadManager,
103101
Map<String, String> clientEnv,
104102
ImmutableMap<String, Optional<Checksum>> knownFileHashes,
105103
KnownFileHashesMode knownFileHashesMode,
106104
ImmutableMap<ModuleKey, String> previouslySelectedYankedVersions,
107105
Optional<Path> vendorDir) {
108106
this.uri = uri;
109-
this.downloadManager = downloadManager;
110107
this.clientEnv = clientEnv;
111108
this.gson =
112109
new GsonBuilder()
@@ -136,9 +133,12 @@ private String constructUrl(String base, String... segments) {
136133

137134
/** Grabs a file from the given URL. Returns {@link Optional#empty} if the file doesn't exist. */
138135
private Optional<byte[]> grabFile(
139-
String url, ExtendedEventHandler eventHandler, boolean useChecksum)
136+
String url,
137+
ExtendedEventHandler eventHandler,
138+
DownloadManager downloadManager,
139+
boolean useChecksum)
140140
throws IOException, InterruptedException {
141-
var maybeContent = doGrabFile(url, eventHandler, useChecksum);
141+
var maybeContent = doGrabFile(downloadManager, url, eventHandler, useChecksum);
142142
if ((knownFileHashesMode == KnownFileHashesMode.USE_AND_UPDATE
143143
|| knownFileHashesMode == KnownFileHashesMode.USE_IMMUTABLE_AND_UPDATE)
144144
&& useChecksum) {
@@ -148,7 +148,10 @@ private Optional<byte[]> grabFile(
148148
}
149149

150150
private Optional<byte[]> doGrabFile(
151-
String rawUrl, ExtendedEventHandler eventHandler, boolean useChecksum)
151+
DownloadManager downloadManager,
152+
String rawUrl,
153+
ExtendedEventHandler eventHandler,
154+
boolean useChecksum)
152155
throws IOException, InterruptedException {
153156
Optional<Checksum> checksum;
154157
if (knownFileHashesMode != KnownFileHashesMode.IGNORE && useChecksum) {
@@ -225,11 +228,13 @@ private Optional<byte[]> doGrabFile(
225228
}
226229

227230
@Override
228-
public Optional<ModuleFile> getModuleFile(ModuleKey key, ExtendedEventHandler eventHandler)
231+
public Optional<ModuleFile> getModuleFile(
232+
ModuleKey key, ExtendedEventHandler eventHandler, DownloadManager downloadManager)
229233
throws IOException, InterruptedException {
230234
String url =
231235
constructUrl(getUrl(), "modules", key.name(), key.version().toString(), "MODULE.bazel");
232-
Optional<byte[]> maybeContent = grabFile(url, eventHandler, /* useChecksum= */ true);
236+
Optional<byte[]> maybeContent =
237+
grabFile(url, eventHandler, downloadManager, /* useChecksum= */ true);
233238
return maybeContent.map(content -> ModuleFile.create(content, url));
234239
}
235240

@@ -276,19 +281,27 @@ private static class GitRepoSourceJson {
276281
* if the file doesn't exist.
277282
*/
278283
private Optional<String> grabJsonFile(
279-
String url, ExtendedEventHandler eventHandler, boolean useChecksum)
284+
String url,
285+
ExtendedEventHandler eventHandler,
286+
DownloadManager downloadManager,
287+
boolean useChecksum)
280288
throws IOException, InterruptedException {
281-
return grabFile(url, eventHandler, useChecksum).map(value -> new String(value, UTF_8));
289+
return grabFile(url, eventHandler, downloadManager, useChecksum)
290+
.map(value -> new String(value, UTF_8));
282291
}
283292

284293
/**
285294
* Grabs a JSON file from the given URL, and returns it as a parsed object with fields in {@code
286295
* T}. Returns {@link Optional#empty} if the file doesn't exist.
287296
*/
288297
private <T> Optional<T> grabJson(
289-
String url, Class<T> klass, ExtendedEventHandler eventHandler, boolean useChecksum)
298+
String url,
299+
Class<T> klass,
300+
ExtendedEventHandler eventHandler,
301+
DownloadManager downloadManager,
302+
boolean useChecksum)
290303
throws IOException, InterruptedException {
291-
Optional<String> jsonString = grabJsonFile(url, eventHandler, useChecksum);
304+
Optional<String> jsonString = grabJsonFile(url, eventHandler, downloadManager, useChecksum);
292305
if (jsonString.isEmpty() || jsonString.get().isBlank()) {
293306
return Optional.empty();
294307
}
@@ -306,10 +319,12 @@ private <T> T parseJson(String jsonString, String url, Class<T> klass) throws IO
306319
}
307320

308321
@Override
309-
public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
322+
public RepoSpec getRepoSpec(
323+
ModuleKey key, ExtendedEventHandler eventHandler, DownloadManager downloadManager)
310324
throws IOException, InterruptedException {
311325
String jsonUrl = getSourceJsonUrl(key);
312-
Optional<String> jsonString = grabJsonFile(jsonUrl, eventHandler, /* useChecksum= */ true);
326+
Optional<String> jsonString =
327+
grabJsonFile(jsonUrl, eventHandler, downloadManager, /* useChecksum= */ true);
313328
if (jsonString.isEmpty()) {
314329
throw new FileNotFoundException(
315330
String.format(
@@ -320,12 +335,14 @@ public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
320335
case "archive" -> {
321336
ArchiveSourceJson typedSourceJson =
322337
parseJson(jsonString.get(), jsonUrl, ArchiveSourceJson.class);
323-
return createArchiveRepoSpec(typedSourceJson, getBazelRegistryJson(eventHandler), key);
338+
return createArchiveRepoSpec(
339+
typedSourceJson, getBazelRegistryJson(eventHandler, downloadManager), key);
324340
}
325341
case "local_path" -> {
326342
LocalPathSourceJson typedSourceJson =
327343
parseJson(jsonString.get(), jsonUrl, LocalPathSourceJson.class);
328-
return createLocalPathRepoSpec(typedSourceJson, getBazelRegistryJson(eventHandler), key);
344+
return createLocalPathRepoSpec(
345+
typedSourceJson, getBazelRegistryJson(eventHandler, downloadManager), key);
329346
}
330347
case "git_repository" -> {
331348
GitRepoSourceJson typedSourceJson =
@@ -343,7 +360,8 @@ private String getSourceJsonUrl(ModuleKey key) {
343360
getUrl(), "modules", key.name(), key.version().toString(), SOURCE_JSON_FILENAME);
344361
}
345362

346-
private Optional<BazelRegistryJson> getBazelRegistryJson(ExtendedEventHandler eventHandler)
363+
private Optional<BazelRegistryJson> getBazelRegistryJson(
364+
ExtendedEventHandler eventHandler, DownloadManager downloadManager)
347365
throws IOException, InterruptedException {
348366
if (bazelRegistryJson == null || bazelRegistryJsonEvents == null) {
349367
synchronized (this) {
@@ -355,6 +373,7 @@ private Optional<BazelRegistryJson> getBazelRegistryJson(ExtendedEventHandler ev
355373
constructUrl(getUrl(), "bazel_registry.json"),
356374
BazelRegistryJson.class,
357375
storedEventHandler,
376+
downloadManager,
358377
/* useChecksum= */ true);
359378
bazelRegistryJsonEvents = storedEventHandler;
360379
}
@@ -484,13 +503,14 @@ private RepoSpec createGitRepoSpec(GitRepoSourceJson sourceJson) {
484503

485504
@Override
486505
public Optional<ImmutableMap<Version, String>> getYankedVersions(
487-
String moduleName, ExtendedEventHandler eventHandler)
506+
String moduleName, ExtendedEventHandler eventHandler, DownloadManager downloadManager)
488507
throws IOException, InterruptedException {
489508
Optional<MetadataJson> metadataJson =
490509
grabJson(
491510
constructUrl(getUrl(), "modules", moduleName, "metadata.json"),
492511
MetadataJson.class,
493512
eventHandler,
513+
downloadManager,
494514
// metadata.json is not immutable
495515
/* useChecksum= */ false);
496516
if (metadataJson.isEmpty()) {

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
3333
import com.google.devtools.build.lib.bazel.repository.PatchUtil;
3434
import com.google.devtools.build.lib.bazel.repository.downloader.Checksum.MissingChecksumException;
35+
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
3536
import com.google.devtools.build.lib.cmdline.Label;
3637
import com.google.devtools.build.lib.cmdline.LabelConstants;
3738
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -99,6 +100,7 @@ public class ModuleFileFunction implements SkyFunction {
99100
private final BazelStarlarkEnvironment starlarkEnv;
100101
private final Path workspaceRoot;
101102
private final ImmutableMap<String, NonRegistryOverride> builtinModules;
103+
@Nullable private DownloadManager downloadManager;
102104

103105
private static final String BZLMOD_REMINDER =
104106
"""
@@ -231,6 +233,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
231233
getModuleFileResult.downloadEventHandler.getPosts()));
232234
}
233235

236+
public void setDownloadManager(DownloadManager downloadManager) {
237+
this.downloadManager = downloadManager;
238+
}
239+
234240
@Nullable
235241
private SkyValue computeForRootModule(
236242
StarlarkSemantics starlarkSemantics, Environment env, SymbolGenerator<?> symbolGenerator)
@@ -620,7 +626,8 @@ private GetModuleFileResult getModuleFile(
620626
StoredEventHandler downloadEventHandler = new StoredEventHandler();
621627
for (Registry registry : registryObjects) {
622628
try {
623-
Optional<ModuleFile> maybeModuleFile = registry.getModuleFile(key, downloadEventHandler);
629+
Optional<ModuleFile> maybeModuleFile =
630+
registry.getModuleFile(key, downloadEventHandler, this.downloadManager);
624631
if (maybeModuleFile.isEmpty()) {
625632
continue;
626633
}

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Registry.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

1818
import com.google.common.collect.ImmutableMap;
19+
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
1920
import com.google.devtools.build.lib.events.ExtendedEventHandler;
2021
import com.google.devtools.build.skyframe.NotComparableSkyValue;
2122
import java.io.IOException;
@@ -31,22 +32,24 @@ public interface Registry extends NotComparableSkyValue {
3132
* Retrieves the contents of the module file of the module identified by {@code key} from the
3233
* registry. Returns {@code Optional.empty()} when the module is not found in this registry.
3334
*/
34-
Optional<ModuleFile> getModuleFile(ModuleKey key, ExtendedEventHandler eventHandler)
35+
Optional<ModuleFile> getModuleFile(
36+
ModuleKey key, ExtendedEventHandler eventHandler, DownloadManager downloadManager)
3537
throws IOException, InterruptedException;
3638

3739
/**
3840
* Retrieves the {@link RepoSpec} object that indicates how the contents of the module identified
3941
* by {@code key} should be materialized as a repo.
4042
*/
41-
RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
43+
RepoSpec getRepoSpec(
44+
ModuleKey key, ExtendedEventHandler eventHandler, DownloadManager downloadManager)
4245
throws IOException, InterruptedException;
4346

4447
/**
4548
* Retrieves yanked versions of the module identified by {@code key.getName()} from the registry.
4649
* Returns {@code Optional.empty()} when the information is not found in the registry.
4750
*/
4851
Optional<ImmutableMap<Version, String>> getYankedVersions(
49-
String moduleName, ExtendedEventHandler eventHandler)
52+
String moduleName, ExtendedEventHandler eventHandler, DownloadManager downloadManager)
5053
throws IOException, InterruptedException;
5154

5255
/**

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFactoryImpl.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,28 +19,21 @@
1919
import com.google.devtools.build.lib.bazel.bzlmod.IndexRegistry.KnownFileHashesMode;
2020
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
2121
import com.google.devtools.build.lib.bazel.repository.downloader.Checksum;
22-
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
2322
import com.google.devtools.build.lib.vfs.Path;
2423
import java.net.URI;
2524
import java.net.URISyntaxException;
2625
import java.util.Map;
2726
import java.util.Optional;
2827
import java.util.function.Supplier;
29-
import javax.annotation.Nullable;
3028

3129
/** Prod implementation of {@link RegistryFactory}. */
3230
public class RegistryFactoryImpl implements RegistryFactory {
33-
@Nullable private DownloadManager downloadManager;
3431
private final Supplier<Map<String, String>> clientEnvironmentSupplier;
3532

3633
public RegistryFactoryImpl(Supplier<Map<String, String>> clientEnvironmentSupplier) {
3734
this.clientEnvironmentSupplier = clientEnvironmentSupplier;
3835
}
3936

40-
public void setDownloadManager(DownloadManager downloadManager) {
41-
this.downloadManager = downloadManager;
42-
}
43-
4437
@Override
4538
public Registry createRegistry(
4639
String url,
@@ -76,7 +69,6 @@ public Registry createRegistry(
7669
};
7770
return new IndexRegistry(
7871
uri,
79-
downloadManager,
8072
clientEnvironmentSupplier.get(),
8173
knownFileHashes,
8274
knownFileHashesMode,

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RepoSpecFunction.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

18+
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
1819
import com.google.devtools.build.lib.events.StoredEventHandler;
1920
import com.google.devtools.build.lib.profiler.Profiler;
2021
import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -32,6 +33,7 @@
3233
* fetching required information from its {@link Registry}.
3334
*/
3435
public class RepoSpecFunction implements SkyFunction {
36+
@Nullable private DownloadManager downloadManager;
3537

3638
@Override
3739
@Nullable
@@ -49,7 +51,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
4951
try (SilentCloseable c =
5052
Profiler.instance()
5153
.profile(ProfilerTask.BZLMOD, () -> "compute repo spec: " + key.getModuleKey())) {
52-
repoSpec = registry.getRepoSpec(key.getModuleKey(), downloadEvents);
54+
repoSpec = registry.getRepoSpec(key.getModuleKey(), downloadEvents, this.downloadManager);
5355
} catch (IOException e) {
5456
throw new RepoSpecException(
5557
ExternalDepsException.withCauseAndMessage(
@@ -63,6 +65,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
6365
repoSpec, RegistryFileDownloadEvent.collectToMap(downloadEvents.getPosts()));
6466
}
6567

68+
public void setDownloadManager(DownloadManager downloadManager) {
69+
this.downloadManager = downloadManager;
70+
}
71+
6672
static final class RepoSpecException extends SkyFunctionException {
6773

6874
RepoSpecException(ExternalDepsException cause) {

0 commit comments

Comments
 (0)