From 330402c66f3befd3ce8c53b73a2184e980147f10 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 30 Apr 2024 14:42:50 +0900 Subject: [PATCH 1/9] Provide a way to automatically delete multipart temporary files Motivation: Armeria does not automatically delete the uploaded files, so users should manually remove the temporary files themselves. It would be useful if we provided some options for how to delete multipart temporary files. Related: #5652 Modifications: - Add `MultipartRemovalStrategy` that is used to determine how to delete multipart files. For now, three options are supported. - NEVER - ON_RESPONSE_COMPLETION - ON_JVM_SHUTDOWN - Add builder methods to server/virtualhost/service builders. Breaking changes: - Multipart temporary files are now automatically removed when a response is fully sent. If you want to keep the existing behavior, use `MultipartRemovalStrategy.NEVER. Result: You can now specify when to remove multipart temporary files using `MultipartRemovalStrategy`. ```java Server .builder() .multipartRemovalStrategy(MultipartRemovalStrategy.ON_RESPONSE_COMPLETION) ``` --- .../armeria/common/DefaultFlagsProvider.java | 6 + .../com/linecorp/armeria/common/Flags.java | 13 ++ .../armeria/common/FlagsProvider.java | 10 ++ .../common/SystemPropertyFlagsProvider.java | 21 +++ .../server/FileAggregatedMultipart.java | 37 +++- ...AbstractAnnotatedServiceConfigSetters.java | 7 + .../server/AbstractServiceBindingBuilder.java | 6 + .../AnnotatedServiceBindingBuilder.java | 5 + ...textPathAnnotatedServiceConfigSetters.java | 12 +- .../ContextPathServiceBindingBuilder.java | 11 +- .../server/DefaultServiceConfigSetters.java | 11 ++ .../server/MultipartRemovalStrategy.java | 41 +++++ .../armeria/server/ServerBuilder.java | 14 ++ .../armeria/server/ServiceBindingBuilder.java | 5 + .../armeria/server/ServiceConfig.java | 29 ++- .../armeria/server/ServiceConfigBuilder.java | 12 +- .../armeria/server/ServiceConfigSetters.java | 7 + ...ualHostAnnotatedServiceBindingBuilder.java | 6 + .../armeria/server/VirtualHostBuilder.java | 18 +- ...textPathAnnotatedServiceConfigSetters.java | 7 + ...lHostContextPathServiceBindingBuilder.java | 6 + .../VirtualHostServiceBindingBuilder.java | 5 + .../linecorp/armeria/common/FlagsTest.java | 36 +++- .../server/MultipartTempFileRemovalTest.java | 111 ++++++++++++ .../armeria/server/ServiceNamingTest.java | 168 ++++-------------- .../linecorp/armeria/server/ServiceTest.java | 3 +- site/src/pages/docs/server-multipart.mdx | 5 +- 27 files changed, 445 insertions(+), 167 deletions(-) create mode 100644 core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java create mode 100644 core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java index be234e7e625..714815e2767 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java @@ -29,6 +29,7 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TlsEngineType; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.server.MultipartRemovalStrategy; import com.linecorp.armeria.server.TransientServiceOption; import io.micrometer.core.instrument.MeterRegistry; @@ -460,6 +461,11 @@ public Path defaultMultipartUploadsLocation() { File.separatorChar + "multipart-uploads"); } + @Override + public MultipartRemovalStrategy defaultMultipartRemovalStrategy() { + return MultipartRemovalStrategy.ON_RESPONSE_COMPLETION; + } + @Override public Sampler requestContextLeakDetectionSampler() { return Sampler.never(); diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index 58e7041189b..c79190218eb 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -58,6 +58,7 @@ import com.linecorp.armeria.internal.common.FlagsLoaded; import com.linecorp.armeria.internal.common.util.SslContextUtil; import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.MultipartRemovalStrategy; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.ServerErrorHandler; import com.linecorp.armeria.server.Service; @@ -399,6 +400,9 @@ private static boolean validateTransportType(TransportType transportType, String private static final Path DEFAULT_MULTIPART_UPLOADS_LOCATION = getValue(FlagsProvider::defaultMultipartUploadsLocation, "defaultMultipartUploadsLocation"); + private static final MultipartRemovalStrategy DEFAULT_MULTIPART_REMOVAL_STRATEGY = + getValue(FlagsProvider::defaultMultipartRemovalStrategy, "defaultMultipartRemovalStrategy"); + private static final Sampler REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER = getValue(FlagsProvider::requestContextLeakDetectionSampler, "requestContextLeakDetectionSampler"); @@ -1442,6 +1446,15 @@ public static Path defaultMultipartUploadsLocation() { return DEFAULT_MULTIPART_UPLOADS_LOCATION; } + /** + * Returns the {@link MultipartRemovalStrategy} that is used to determine how to remove the uploaded files + * from {@code multipart/form-data}. + */ + @UnstableApi + public static MultipartRemovalStrategy defaultMultipartRemovalStrategy() { + return DEFAULT_MULTIPART_REMOVAL_STRATEGY; + } + /** * Returns whether to allow double dots ({@code ..}) in a request path query string. * diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index e2b606fc1c2..5902279b06c 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -45,6 +45,7 @@ import com.linecorp.armeria.common.util.TlsEngineType; import com.linecorp.armeria.common.util.TransportType; import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.MultipartRemovalStrategy; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.Service; import com.linecorp.armeria.server.ServiceRequestContext; @@ -1095,6 +1096,15 @@ default Path defaultMultipartUploadsLocation() { return null; } + /** + * Returns the {@link MultipartRemovalStrategy} that is used to determine how to remove the uploaded files + * from {@code multipart/form-data}. + */ + @Nullable + default MultipartRemovalStrategy defaultMultipartRemovalStrategy() { + return null; + } + /** * Returns the {@link Sampler} that determines whether to trace the stack trace of request contexts leaks * and how frequently to keeps stack trace. A sampled exception will have the stack trace while the others diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index d1b3411fbd1..b961d670eb5 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -41,6 +41,7 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TlsEngineType; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.server.MultipartRemovalStrategy; import com.linecorp.armeria.server.TransientServiceOption; /** @@ -473,6 +474,26 @@ public Path defaultMultipartUploadsLocation() { return getAndParse("defaultMultipartUploadsLocation", Paths::get); } + @Nullable + @Override + public MultipartRemovalStrategy defaultMultipartRemovalStrategy() { + final String multipartRemovalStrategy = getNormalized("defaultMultipartRemovalStrategy"); + if (multipartRemovalStrategy == null) { + return null; + } + switch (multipartRemovalStrategy) { + case "never": + return MultipartRemovalStrategy.NEVER; + case "on_response_completion": + return MultipartRemovalStrategy.ON_RESPONSE_COMPLETION; + case "on_jvm_shutdown": + return MultipartRemovalStrategy.ON_JVM_SHUTDOWN; + default: + throw new IllegalArgumentException( + multipartRemovalStrategy + " isn't MultipartRemovalStrategy"); + } + } + @Override public Sampler requestContextLeakDetectionSampler() { final String spec = getNormalized("requestContextLeakDetectionSampler"); diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java index bae2dd4e643..ce0a1952f5a 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -26,6 +26,9 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; @@ -39,6 +42,8 @@ import io.netty.channel.EventLoop; public final class FileAggregatedMultipart { + private static final Logger logger = LoggerFactory.getLogger(FileAggregatedMultipart.class); + private final ListMultimap params; private final ListMultimap files; @@ -72,7 +77,7 @@ public static CompletableFuture aggregateMultipart(Serv return resolveTmpFile(incompleteDir, filename, executor).thenCompose(path -> { return bodyPart.writeTo(path, eventLoop, executor).thenCompose(ignore -> { final Path completeDir = destination.resolve("complete"); - return moveFile(path, completeDir, executor); + return moveFile(path, completeDir, executor, ctx); }).thenApply(completePath -> MultipartFile.of(name, filename, completePath.toFile(), bodyPart.headers())); }); @@ -102,19 +107,43 @@ public static CompletableFuture aggregateMultipart(Serv } private static CompletableFuture moveFile(Path file, Path targetDirectory, - ExecutorService blockingExecutorService) { + ExecutorService blockingExecutorService, + ServiceRequestContext ctx) { return CompletableFuture.supplyAsync(() -> { try { Files.createDirectories(targetDirectory); // Avoid name duplication, create new file at target place and replace it. - return Files.move(file, Files.createTempFile(targetDirectory, null, ".multipart"), - StandardCopyOption.REPLACE_EXISTING); + final Path tempFile = createRemovableTempFile(targetDirectory, blockingExecutorService, ctx); + return Files.move(file, tempFile, StandardCopyOption.REPLACE_EXISTING); } catch (IOException e) { throw new UncheckedIOException(e); } }, blockingExecutorService); } + private static Path createRemovableTempFile(Path targetDirectory, + ExecutorService blockingExecutorService, + ServiceRequestContext ctx) throws IOException { + final Path tempFile = Files.createTempFile(targetDirectory, null, ".multipart"); + switch (ctx.config().multipartRemovalStrategy()) { + case NEVER: + break; + case ON_RESPONSE_COMPLETION: + ctx.log().whenComplete().thenAcceptAsync(unused -> { + try { + Files.deleteIfExists(tempFile); + } catch (IOException e) { + logger.warn("Failed to delete a temporary file: {}", tempFile, e); + } + }, blockingExecutorService); + break; + case ON_JVM_SHUTDOWN: + tempFile.toFile().deleteOnExit(); + break; + } + return tempFile; + } + private static CompletableFuture resolveTmpFile(Path directory, String filename, ExecutorService blockingExecutorService) { diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java index 80ca978f5a3..d800d2ad213 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java @@ -282,6 +282,13 @@ public AbstractAnnotatedServiceConfigSetters multipartUploadsLocation(Path multi return this; } + @UnstableApi + @Override + public AbstractAnnotatedServiceConfigSetters multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + defaultServiceConfigSetters.multipartRemovalStrategy(removalStrategy); + return this; + } + @Override public ServiceConfigSetters serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { defaultServiceConfigSetters.serviceWorkerGroup(serviceWorkerGroup, shutdownOnStop); diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java index 9506174428b..17e8f5d2ee2 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java @@ -177,6 +177,12 @@ public AbstractServiceBindingBuilder multipartUploadsLocation(Path multipartUplo return this; } + @Override + public AbstractServiceBindingBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + defaultServiceConfigSetters.multipartRemovalStrategy(removalStrategy); + return this; + } + @Override public AbstractServiceBindingBuilder serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { diff --git a/core/src/main/java/com/linecorp/armeria/server/AnnotatedServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/AnnotatedServiceBindingBuilder.java index d232a8855d8..b2053f33c0d 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AnnotatedServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/AnnotatedServiceBindingBuilder.java @@ -224,6 +224,11 @@ public AnnotatedServiceBindingBuilder multipartUploadsLocation(Path multipartUpl return (AnnotatedServiceBindingBuilder) super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public AnnotatedServiceBindingBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + return (AnnotatedServiceBindingBuilder) super.multipartRemovalStrategy(removalStrategy); + } + @Override public AnnotatedServiceBindingBuilder requestIdGenerator( Function requestIdGenerator) { diff --git a/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java index 783e5efcd15..4e8c8bccf64 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java @@ -51,9 +51,9 @@ public final class ContextPathAnnotatedServiceConfigSetters * Registers the given service to {@link ContextPathServicesBuilder} and returns the parent object. * * @param service annotated service object to handle incoming requests matching path prefix, which - * can be configured through {@link AnnotatedServiceBindingBuilder#pathPrefix(String)}. - * If path prefix is not set then this service is registered to handle requests matching - * {@code /} + * can be configured through {@link AnnotatedServiceBindingBuilder#pathPrefix(String)}. + * If path prefix is not set then this service is registered to handle requests matching + * {@code /} */ @Override public ContextPathServicesBuilder build(Object service) { @@ -250,6 +250,12 @@ public ContextPathAnnotatedServiceConfigSetters multipartUploadsLocation( super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public ContextPathAnnotatedServiceConfigSetters multipartRemovalStrategy( + MultipartRemovalStrategy removalStrategy) { + return (ContextPathAnnotatedServiceConfigSetters) super.multipartRemovalStrategy(removalStrategy); + } + @Override public ContextPathAnnotatedServiceConfigSetters requestIdGenerator( Function requestIdGenerator) { diff --git a/core/src/main/java/com/linecorp/armeria/server/ContextPathServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/ContextPathServiceBindingBuilder.java index a2060b6df5c..0a8a7f85ac2 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ContextPathServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/ContextPathServiceBindingBuilder.java @@ -161,17 +161,20 @@ public ContextPathServiceBindingBuilder requestAutoAbortDelay(Duration delay) { } @Override - public ContextPathServiceBindingBuilder requestAutoAbortDelayMillis( - long delayMillis) { + public ContextPathServiceBindingBuilder requestAutoAbortDelayMillis(long delayMillis) { return (ContextPathServiceBindingBuilder) super.requestAutoAbortDelayMillis(delayMillis); } @Override - public ContextPathServiceBindingBuilder multipartUploadsLocation( - Path multipartUploadsLocation) { + public ContextPathServiceBindingBuilder multipartUploadsLocation(Path multipartUploadsLocation) { return (ContextPathServiceBindingBuilder) super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public ContextPathServiceBindingBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + return (ContextPathServiceBindingBuilder) super.multipartRemovalStrategy(removalStrategy); + } + @Override public ContextPathServiceBindingBuilder serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { diff --git a/core/src/main/java/com/linecorp/armeria/server/DefaultServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/DefaultServiceConfigSetters.java index b5d1e948a9e..bd981d623dc 100644 --- a/core/src/main/java/com/linecorp/armeria/server/DefaultServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/DefaultServiceConfigSetters.java @@ -79,6 +79,8 @@ final class DefaultServiceConfigSetters implements ServiceConfigSetters { @Nullable private Path multipartUploadsLocation; @Nullable + private MultipartRemovalStrategy multipartRemovalStrategy; + @Nullable private EventLoopGroup serviceWorkerGroup; @Nullable private ServiceErrorHandler serviceErrorHandler; @@ -237,6 +239,12 @@ public ServiceConfigSetters multipartUploadsLocation(Path multipartUploadsLocati return this; } + @Override + public ServiceConfigSetters multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + this.multipartRemovalStrategy = requireNonNull(removalStrategy, "removalStrategy"); + return this; + } + @Override public ServiceConfigSetters serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { @@ -377,6 +385,9 @@ ServiceConfigBuilder toServiceConfigBuilder(Route route, String contextPath, Htt if (multipartUploadsLocation != null) { serviceConfigBuilder.multipartUploadsLocation(multipartUploadsLocation); } + if (multipartRemovalStrategy != null) { + serviceConfigBuilder.multipartRemovalStrategy(multipartRemovalStrategy); + } if (serviceWorkerGroup != null) { serviceConfigBuilder.serviceWorkerGroup(serviceWorkerGroup, false); // Set the serviceWorkerGroup as false because it's shut down in ShutdownSupport. diff --git a/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java b/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java new file mode 100644 index 00000000000..b72c6a28f5b --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java @@ -0,0 +1,41 @@ +/* + * Copyright 2024 LINE Corporation + * + * LINE Corporation licenses this file to you 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: + * + * https://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.linecorp.armeria.server; + +import com.linecorp.armeria.common.annotation.UnstableApi; + +/** + * Specifies when to remove the temporary files created for multipart requests. + */ +@UnstableApi +public enum MultipartRemovalStrategy { + /** + * Never remove the temporary files. + * + *

Warning: This option may cause a disk space leak if the temporary files are not + * removed manually. + */ + NEVER, + /** + * Remove the temporary files after the response is fully sent. + */ + ON_RESPONSE_COMPLETION, + /** + * Remove the temporary files when the JVM is shutting down. + */ + ON_JVM_SHUTDOWN +} diff --git a/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java b/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java index 0ab6587642e..45697b1341a 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java @@ -255,6 +255,7 @@ public final class ServerBuilder implements TlsSetters, ServiceConfigsBuilder { virtualHostTemplate.successFunction(SuccessFunction.ofDefault()); virtualHostTemplate.requestAutoAbortDelayMillis(0); virtualHostTemplate.multipartUploadsLocation(Flags.defaultMultipartUploadsLocation()); + virtualHostTemplate.multipartRemovalStrategy(Flags.defaultMultipartRemovalStrategy()); virtualHostTemplate.requestIdGenerator(routingContext -> RequestId.random()); } @@ -953,6 +954,19 @@ public ServerBuilder multipartUploadsLocation(Path path) { return this; } + /** + * Sets the {@link MultipartRemovalStrategy} that determines when to remove temporary files created + * for multipart requests. + * If not set, {@link MultipartRemovalStrategy#ON_RESPONSE_COMPLETION} is used by default. + * + */ + @UnstableApi + public ServerBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + requireNonNull(removalStrategy, "removalStrategy"); + virtualHostTemplate.multipartRemovalStrategy(removalStrategy); + return this; + } + /** * Sets the {@link ScheduledExecutorService} dedicated to the execution of blocking tasks or invocations. * If not set, {@linkplain CommonPools#blockingTaskExecutor() the common pool} is used. diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/ServiceBindingBuilder.java index 60e77711e4e..a1dbea08caf 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceBindingBuilder.java @@ -293,6 +293,11 @@ public ServiceBindingBuilder multipartUploadsLocation(Path multipartUploadsLocat return (ServiceBindingBuilder) super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public ServiceBindingBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + return (ServiceBindingBuilder) super.multipartRemovalStrategy(removalStrategy); + } + @Override public ServiceBindingBuilder serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceConfig.java b/core/src/main/java/com/linecorp/armeria/server/ServiceConfig.java index 0a27ab662eb..081fe770778 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceConfig.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceConfig.java @@ -81,6 +81,7 @@ public final class ServiceConfig { private final long requestAutoAbortDelayMillis; private final Path multipartUploadsLocation; + private final MultipartRemovalStrategy multipartRemovalStrategy; private final EventLoopGroup serviceWorkerGroup; private final List shutdownSupports; @@ -98,7 +99,8 @@ public final class ServiceConfig { boolean verboseResponses, AccessLogWriter accessLogWriter, BlockingTaskExecutor blockingTaskExecutor, SuccessFunction successFunction, long requestAutoAbortDelayMillis, - Path multipartUploadsLocation, EventLoopGroup serviceWorkerGroup, + Path multipartUploadsLocation, MultipartRemovalStrategy multipartRemovalStrategy, + EventLoopGroup serviceWorkerGroup, List shutdownSupports, HttpHeaders defaultHeaders, Function requestIdGenerator, @@ -107,8 +109,8 @@ public final class ServiceConfig { requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, extractTransientServiceOptions(service), blockingTaskExecutor, successFunction, requestAutoAbortDelayMillis, - multipartUploadsLocation, serviceWorkerGroup, shutdownSupports, defaultHeaders, - requestIdGenerator, serviceErrorHandler, contextHook); + multipartUploadsLocation, multipartRemovalStrategy, serviceWorkerGroup, shutdownSupports, + defaultHeaders, requestIdGenerator, serviceErrorHandler, contextHook); } /** @@ -123,7 +125,7 @@ private ServiceConfig(@Nullable VirtualHost virtualHost, Route route, BlockingTaskExecutor blockingTaskExecutor, SuccessFunction successFunction, long requestAutoAbortDelayMillis, - Path multipartUploadsLocation, + Path multipartUploadsLocation, MultipartRemovalStrategy multipartRemovalStrategy, EventLoopGroup serviceWorkerGroup, List shutdownSupports, HttpHeaders defaultHeaders, Function requestIdGenerator, @@ -145,6 +147,7 @@ private ServiceConfig(@Nullable VirtualHost virtualHost, Route route, this.successFunction = requireNonNull(successFunction, "successFunction"); this.requestAutoAbortDelayMillis = requestAutoAbortDelayMillis; this.multipartUploadsLocation = requireNonNull(multipartUploadsLocation, "multipartUploadsLocation"); + this.multipartRemovalStrategy = requireNonNull(multipartRemovalStrategy, "multipartRemovalStrategy"); this.serviceWorkerGroup = requireNonNull(serviceWorkerGroup, "serviceWorkerGroup"); this.shutdownSupports = ImmutableList.copyOf(requireNonNull(shutdownSupports, "shutdownSupports")); this.defaultHeaders = defaultHeaders; @@ -192,7 +195,8 @@ ServiceConfig withVirtualHost(VirtualHost virtualHost) { defaultServiceNaming, requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, transientServiceOptions, blockingTaskExecutor, successFunction, requestAutoAbortDelayMillis, - multipartUploadsLocation, serviceWorkerGroup, shutdownSupports, defaultHeaders, + multipartUploadsLocation, multipartRemovalStrategy, serviceWorkerGroup, + shutdownSupports, defaultHeaders, requestIdGenerator, serviceErrorHandler, contextHook); } @@ -203,7 +207,8 @@ ServiceConfig withDecoratedService(Function defaultRequestIdGenerator, @@ -388,6 +397,7 @@ ServiceConfig build(ServiceNaming defaultServiceNaming, successFunction != null ? successFunction : defaultSuccessFunction, requestAutoAbortDelayMillis, multipartUploadsLocation != null ? multipartUploadsLocation : defaultMultipartUploadsLocation, + multipartRemovalStrategy != null ? multipartRemovalStrategy : defaultMultipartRemovalStrategy, serviceWorkerGroup != null ? serviceWorkerGroup : defaultServiceWorkerGroup, ImmutableList.copyOf(shutdownSupports), mergeDefaultHeaders(virtualHostDefaultHeaders.toBuilder(), defaultHeaders.build()), diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/ServiceConfigSetters.java index a1066477998..95579980c20 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceConfigSetters.java @@ -211,6 +211,13 @@ ServiceConfigSetters blockingTaskExecutor(BlockingTaskExecutor blockingTaskExecu @UnstableApi ServiceConfigSetters multipartUploadsLocation(Path multipartUploadsLocation); + /** + * Sets the {@link MultipartRemovalStrategy} that determines when to remove temporary files created + * for multipart requests. + */ + @UnstableApi + ServiceConfigSetters multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy); + /** * Sets a {@linkplain EventLoopGroup worker group} to be used when serving a {@link Service}. * diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostAnnotatedServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostAnnotatedServiceBindingBuilder.java index 2e903e9ae93..367d2149cce 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostAnnotatedServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostAnnotatedServiceBindingBuilder.java @@ -231,6 +231,12 @@ public VirtualHostAnnotatedServiceBindingBuilder multipartUploadsLocation(Path m super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public VirtualHostAnnotatedServiceBindingBuilder multipartRemovalStrategy( + MultipartRemovalStrategy removalStrategy) { + return (VirtualHostAnnotatedServiceBindingBuilder) super.multipartRemovalStrategy(removalStrategy); + } + @Override public VirtualHostAnnotatedServiceBindingBuilder requestIdGenerator( Function requestIdGenerator) { diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java index 88c5c98a2ba..cf480e850c5 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java @@ -165,6 +165,8 @@ public final class VirtualHostBuilder implements TlsSetters, ServiceConfigsBuild @Nullable private Path multipartUploadsLocation; @Nullable + private MultipartRemovalStrategy multipartRemovalStrategy; + @Nullable private EventLoopGroup serviceWorkerGroup; @Nullable private Function requestIdGenerator; @@ -1191,6 +1193,11 @@ Path multipartUploadsLocation() { return multipartUploadsLocation; } + @UnstableApi + public void multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + multipartRemovalStrategy = requireNonNull(removalStrategy, "removalStrategy"); + } + /** * Sets the {@link Function} which generates a {@link RequestId}. * If not set, the value set via {@link ServerBuilder#requestIdGenerator(Function)} is used. @@ -1353,6 +1360,9 @@ VirtualHost build(VirtualHostBuilder template, DependencyInjector dependencyInje final Path multipartUploadsLocation = this.multipartUploadsLocation != null ? this.multipartUploadsLocation : template.multipartUploadsLocation; + final MultipartRemovalStrategy multipartRemovalStrategy = + this.multipartRemovalStrategy != null ? + this.multipartRemovalStrategy : template.multipartRemovalStrategy; final HttpHeaders defaultHeaders = mergeDefaultHeaders(template.defaultHeaders, this.defaultHeaders.build()); @@ -1382,6 +1392,7 @@ VirtualHost build(VirtualHostBuilder template, DependencyInjector dependencyInje assert blockingTaskExecutor != null; assert successFunction != null; assert multipartUploadsLocation != null; + assert multipartRemovalStrategy != null; assert requestIdGenerator != null; final List serviceConfigs = getServiceConfigSetters(template) @@ -1401,7 +1412,8 @@ VirtualHost build(VirtualHostBuilder template, DependencyInjector dependencyInje return cfgBuilder.build(defaultServiceNaming, requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, blockingTaskExecutor, successFunction, requestAutoAbortDelayMillis, - multipartUploadsLocation, serviceWorkerGroup, defaultHeaders, + multipartUploadsLocation, multipartRemovalStrategy, + serviceWorkerGroup, defaultHeaders, requestIdGenerator, defaultErrorHandler, unhandledExceptionsReporter, baseContextPath, contextHook); }).collect(toImmutableList()); @@ -1410,8 +1422,8 @@ VirtualHost build(VirtualHostBuilder template, DependencyInjector dependencyInje new ServiceConfigBuilder(RouteBuilder.FALLBACK_ROUTE, "/", FallbackService.INSTANCE) .build(defaultServiceNaming, requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, blockingTaskExecutor, successFunction, - requestAutoAbortDelayMillis, multipartUploadsLocation, serviceWorkerGroup, - defaultHeaders, requestIdGenerator, + requestAutoAbortDelayMillis, multipartUploadsLocation, multipartRemovalStrategy, + serviceWorkerGroup, defaultHeaders, requestIdGenerator, defaultErrorHandler, unhandledExceptionsReporter, "/", contextHook); final ImmutableList.Builder builder = ImmutableList.builder(); diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathAnnotatedServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathAnnotatedServiceConfigSetters.java index e58dc62d644..c94c59a122e 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathAnnotatedServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathAnnotatedServiceConfigSetters.java @@ -265,6 +265,13 @@ public VirtualHostContextPathAnnotatedServiceConfigSetters multipartUploadsLocat super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public VirtualHostContextPathAnnotatedServiceConfigSetters multipartRemovalStrategy( + MultipartRemovalStrategy removalStrategy) { + return (VirtualHostContextPathAnnotatedServiceConfigSetters) super.multipartRemovalStrategy( + removalStrategy); + } + @Override public VirtualHostContextPathAnnotatedServiceConfigSetters requestIdGenerator( Function requestIdGenerator) { diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServiceBindingBuilder.java index 7d930cf5560..a4281ac24c4 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServiceBindingBuilder.java @@ -180,6 +180,12 @@ public VirtualHostContextPathServiceBindingBuilder multipartUploadsLocation( super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public VirtualHostContextPathServiceBindingBuilder multipartRemovalStrategy( + MultipartRemovalStrategy removalStrategy) { + return (VirtualHostContextPathServiceBindingBuilder) super.multipartRemovalStrategy(removalStrategy); + } + @Override public VirtualHostContextPathServiceBindingBuilder serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostServiceBindingBuilder.java index 8a8f10464eb..8df0a04f8a8 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostServiceBindingBuilder.java @@ -317,6 +317,11 @@ public VirtualHostServiceBindingBuilder multipartUploadsLocation(Path multipartU return (VirtualHostServiceBindingBuilder) super.multipartUploadsLocation(multipartUploadsLocation); } + @Override + public VirtualHostServiceBindingBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + return (VirtualHostServiceBindingBuilder) super.multipartRemovalStrategy(removalStrategy); + } + @Override public VirtualHostServiceBindingBuilder serviceWorkerGroup(EventLoopGroup serviceWorkerGroup, boolean shutdownOnStop) { diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index 2750562faf2..ff226943fe4 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -49,6 +49,7 @@ import com.linecorp.armeria.common.util.Sampler; import com.linecorp.armeria.common.util.TlsEngineType; import com.linecorp.armeria.common.util.TransportType; +import com.linecorp.armeria.server.MultipartRemovalStrategy; import com.linecorp.armeria.server.TransientServiceOption; import io.netty.channel.epoll.Epoll; @@ -57,12 +58,13 @@ class FlagsTest { private static final String osName = Ascii.toLowerCase(System.getProperty("os.name")); + private FlagsClassLoader flagsClassLoader; private Class flags; @BeforeEach void reloadFlags() throws ClassNotFoundException { - final FlagsClassLoader classLoader = new FlagsClassLoader(); - flags = classLoader.loadClass(Flags.class.getCanonicalName()); + flagsClassLoader = new FlagsClassLoader(); + flags = flagsClassLoader.loadClass(Flags.class.getCanonicalName()); } /** @@ -180,7 +182,7 @@ void invalidBooleanSystemPropertyFlag() throws Throwable { @Test @SetSystemProperty(key = "com.linecorp.armeria.preferredIpV4Addresses", - value = "211.111.111.111,10.0.0.0/8,192.168.1.0/24") + value = "211.111.111.111,10.0.0.0/8,192.168.1.0/24") void preferredIpV4Addresses() throws Throwable { final Lookup lookup = MethodHandles.publicLookup(); final MethodHandle method = @@ -197,7 +199,7 @@ void preferredIpV4Addresses() throws Throwable { @Test @SetSystemProperty(key = "com.linecorp.armeria.preferredIpV4Addresses", - value = "211.111.111.111,10.0.0.0/40") + value = "211.111.111.111,10.0.0.0/40") void someOfPreferredIpV4AddressesIsInvalid() throws Throwable { // 10.0.0.0/40 is invalid cidr final Lookup lookup = MethodHandles.publicLookup(); @@ -263,6 +265,21 @@ void invalidSystemPropertyRequestContextLeakDetectionSampler() throws Exception .isEqualTo(Sampler.never()); } + @Test + void defaultMultipartRemovalStrategy() throws Throwable { + assertThat(Flags.defaultMultipartRemovalStrategy()) + .isEqualTo(MultipartRemovalStrategy.ON_RESPONSE_COMPLETION); + } + + @Test + @SetSystemProperty(key = "com.linecorp.armeria.defaultMultipartRemovalStrategy", value = "NEVER") + void overrideMultipartRemovalStrategy() throws Throwable { + final Method method = flags.getDeclaredMethod("defaultMultipartRemovalStrategy"); + assertThat(method.invoke(flags)) + .usingRecursiveComparison() + .isEqualTo(MultipartRemovalStrategy.NEVER); + } + @Test void testApiConsistencyBetweenFlagsAndFlagsProvider() { //Check method consistency between Flags and FlagsProvider excluding deprecated methods @@ -287,9 +304,10 @@ void testApiConsistencyBetweenFlagsAndFlagsProvider() { private ObjectAssert assertFlags(String flagsMethod) throws Throwable { final Lookup lookup = MethodHandles.publicLookup(); + final Class rtype = flagsClassLoader.loadClass(Flags.class.getMethod(flagsMethod).getReturnType() + .getCanonicalName()); final MethodHandle method = - lookup.findStatic(flags, flagsMethod, MethodType.methodType( - Flags.class.getMethod(flagsMethod).getReturnType())); + lookup.findStatic(flags, flagsMethod, MethodType.methodType(rtype)); return assertThat(method.invoke()); } @@ -322,7 +340,11 @@ public Class loadClass(String name) throws ClassNotFoundException { input.close(); final byte[] classData = buffer.toByteArray(); - return defineClass(name, classData, 0, classData.length); + final Class aClass = defineClass(name, classData, 0, classData.length); + if (name.contains("Multipart")) { + System.out.println("Loading class: " + name + ", class: " + aClass); + } + return aClass; } catch (IOException e) { Exceptions.throwUnsafely(e); } diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java new file mode 100644 index 00000000000..5772a579c31 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java @@ -0,0 +1,111 @@ +/* + * Copyright 2024 LINE Corporation + * + * LINE Corporation licenses this file to you 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: + * + * https://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.linecorp.armeria.internal.server; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.linecorp.armeria.client.BlockingWebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.ContentDisposition; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.MediaTypeNames; +import com.linecorp.armeria.common.multipart.BodyPart; +import com.linecorp.armeria.common.multipart.Multipart; +import com.linecorp.armeria.server.MultipartRemovalStrategy; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.server.annotation.Blocking; +import com.linecorp.armeria.server.annotation.Consumes; +import com.linecorp.armeria.server.annotation.Param; +import com.linecorp.armeria.server.annotation.Path; +import com.linecorp.armeria.server.annotation.Post; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class MultipartTempFileRemovalTest { + + private static final Logger logger = LoggerFactory.getLogger(MultipartTempFileRemovalTest.class); + + @RegisterExtension + static final ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) throws Exception { + sb.annotatedService() + .pathPrefix("/never") + .multipartRemovalStrategy(MultipartRemovalStrategy.NEVER) + .build(new TestMultipartService()); + sb.annotatedService() + .pathPrefix("/on_jvm_shutdown") + .multipartRemovalStrategy(MultipartRemovalStrategy.ON_JVM_SHUTDOWN) + .build(new TestMultipartService()); + sb.annotatedService() + .pathPrefix("/default") + .build(new TestMultipartService()); + } + }; + + @CsvSource({ "/never", "/default" }) + @ParameterizedTest + void testRemovalStrategy(String type) throws Exception { + final BlockingWebClient client = server.blockingWebClient(); + final ContentDisposition contentDisposition = ContentDisposition.of("form-data", "file1", "file1.txt"); + final Multipart multipart = Multipart.of(BodyPart.of(contentDisposition, "file1 content")); + final AggregatedHttpResponse response = client.execute(multipart.toHttpRequest(type + "/upload")); + server.requestContextCaptor().take().log().whenComplete().join(); + final java.nio.file.Path path = Paths.get(response.contentUtf8()); + if ("/never".equals(type)) { + assertThat(Files.exists(path)).isTrue(); + Files.delete(path); + } else { + assertThat(Files.exists(path)).isFalse(); + } + } + + @Disabled("Enable this test to manually check if the temporary file is removed after the JVM shuts down.") + @CsvSource("/on_jvm_shutdown") + @ParameterizedTest + void testOnJvmShutDown(String type) throws Exception { + final BlockingWebClient client = server.blockingWebClient(); + final ContentDisposition contentDisposition = ContentDisposition.of("form-data", "file1", "file1.txt"); + final Multipart multipart = Multipart.of(BodyPart.of(contentDisposition, "file1 content")); + final AggregatedHttpResponse response = client.execute(multipart.toHttpRequest(type + "/upload")); + server.requestContextCaptor().take().log().whenComplete().join(); + final java.nio.file.Path path = Paths.get(response.contentUtf8()); + assertThat(Files.exists(path)).isTrue(); + logger.info("Please check if the temporary file is removed after the JVM shuts down: {}", path); + } + + @Consumes(MediaTypeNames.MULTIPART_FORM_DATA) + private static class TestMultipartService { + @Blocking + @Post + @Path("/upload") + public HttpResponse upload(@Param File file1) throws IOException { + return HttpResponse.of(file1.getPath()); + } + } +} diff --git a/core/src/test/java/com/linecorp/armeria/server/ServiceNamingTest.java b/core/src/test/java/com/linecorp/armeria/server/ServiceNamingTest.java index 9fa6e530070..51cd472a3cb 100644 --- a/core/src/test/java/com/linecorp/armeria/server/ServiceNamingTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/ServiceNamingTest.java @@ -40,15 +40,8 @@ class ServiceNamingTest { @Test void fullTypeName_topClass() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), HealthCheckService.builder().build(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(HealthCheckService.builder().build(), + ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.fullTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(HealthCheckService.class.getName()); @@ -57,15 +50,7 @@ void fullTypeName_topClass() { @Test void fullTypeName_nestedClass() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new NestedClass(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new NestedClass(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.fullTypeName().serviceName(ctx); assertThat(serviceName) @@ -75,15 +60,7 @@ void fullTypeName_nestedClass() { @Test void fullTypeName_trimTrailingDollarSign() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new TrailingDollarSign$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new TrailingDollarSign$(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.fullTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(ServiceNamingTest.class.getName() + "$TrailingDollarSign"); @@ -92,15 +69,8 @@ void fullTypeName_trimTrailingDollarSign() { @Test void fullTypeName_trimTrailingDollarSignMany() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new TrailingDollarSign$$$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new TrailingDollarSign$$$(), + ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.fullTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(ServiceNamingTest.class.getName() + "$TrailingDollarSign"); @@ -109,15 +79,7 @@ void fullTypeName_trimTrailingDollarSignMany() { @Test void fullTypeName_trimTrailingDollarSignOnly() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new $$$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new $$$(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.fullTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(ServiceNamingTest.class.getName()); @@ -126,15 +88,8 @@ void fullTypeName_trimTrailingDollarSignOnly() { @Test void simpleTypeName_topClass() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), HealthCheckService.builder().build(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(HealthCheckService.builder().build(), + ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.simpleTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(HealthCheckService.class.getSimpleName()); @@ -143,15 +98,7 @@ void simpleTypeName_topClass() { @Test void simpleTypeName_nestedClass() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new NestedClass(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new NestedClass(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.simpleTypeName().serviceName(ctx); assertThat(serviceName) @@ -161,15 +108,7 @@ void simpleTypeName_nestedClass() { @Test void simpleTypeName_trimTrailingDollarSign() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new TrailingDollarSign$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new TrailingDollarSign$(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.simpleTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(ServiceNamingTest.class.getSimpleName() + "$TrailingDollarSign"); @@ -178,15 +117,8 @@ void simpleTypeName_trimTrailingDollarSign() { @Test void simpleTypeName_trimTrailingDollarSignMany() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new TrailingDollarSign$$$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new TrailingDollarSign$$$(), + ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.simpleTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(ServiceNamingTest.class.getSimpleName() + "$TrailingDollarSign"); @@ -195,15 +127,7 @@ void simpleTypeName_trimTrailingDollarSignMany() { @Test void simpleTypeName_trimTrailingDollarSignOnly() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new $$$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new $$$(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.simpleTypeName().serviceName(ctx); assertThat(serviceName).isEqualTo(ServiceNamingTest.class.getSimpleName()); @@ -212,15 +136,8 @@ void simpleTypeName_trimTrailingDollarSignOnly() { @Test void shorten_topClass() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), HealthCheckService.builder().build(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(HealthCheckService.builder().build(), + ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.shorten().serviceName(ctx); assertThat(serviceName).isEqualTo("c.l.a.s.h." + HealthCheckService.class.getSimpleName()); @@ -229,15 +146,7 @@ void shorten_topClass() { @Test void shorten_nestedClass() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new NestedClass(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new NestedClass(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.shorten().serviceName(ctx); assertThat(serviceName).isEqualTo("c.l.a.s." + ServiceNamingTest.class.getSimpleName() + '$' + @@ -247,15 +156,7 @@ void shorten_nestedClass() { @Test void shorten_trimTrailingDollarSign() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new TrailingDollarSign$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new TrailingDollarSign$(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.shorten().serviceName(ctx); assertThat(serviceName) @@ -265,15 +166,8 @@ void shorten_trimTrailingDollarSign() { @Test void shorten_trimTrailingDollarSignMany() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new TrailingDollarSign$$$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new TrailingDollarSign$$$(), + ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.shorten().serviceName(ctx); assertThat(serviceName) @@ -283,21 +177,25 @@ void shorten_trimTrailingDollarSignMany() { @Test void shorten_trimTrailingDollarSignOnly() { final ServiceRequestContext ctx = mock(ServiceRequestContext.class); - final ServiceConfig config = - new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), new $$$(), - null, null, ServiceNaming.fullTypeName(), 0, 0, false, - AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), - routingCtx -> RequestId.of(1L), - ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + final ServiceConfig config = newServiceConfig(new $$$(), ServiceNaming.fullTypeName()); when(ctx.config()).thenReturn(config); final String serviceName = ServiceNaming.shorten().serviceName(ctx); assertThat(serviceName) .isEqualTo("c.l.a.s." + ServiceNamingTest.class.getSimpleName()); } + private static ServiceConfig newServiceConfig(HttpService httpService, ServiceNaming serviceNaming) { + return new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), httpService, + null, null, serviceNaming, 0, 0, false, + AccessLogWriter.common(), CommonPools.blockingTaskExecutor(), + SuccessFunction.always(), + 0, Files.newTemporaryFolder().toPath(), + MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, CommonPools.workerGroup(), + ImmutableList.of(), HttpHeaders.of(), + routingCtx -> RequestId.of(1L), + ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); + } + private static final class NestedClass implements HttpService { @Override diff --git a/core/src/test/java/com/linecorp/armeria/server/ServiceTest.java b/core/src/test/java/com/linecorp/armeria/server/ServiceTest.java index 659308bae20..9b67ac5dc3d 100644 --- a/core/src/test/java/com/linecorp/armeria/server/ServiceTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/ServiceTest.java @@ -65,7 +65,8 @@ private static void assertDecoration(FooService inner, HttpService outer) throws AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), SuccessFunction.always(), - 0, Files.newTemporaryFolder().toPath(), CommonPools.workerGroup(), + 0, Files.newTemporaryFolder().toPath(), + MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), ctx -> RequestId.of(1L), ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK); diff --git a/site/src/pages/docs/server-multipart.mdx b/site/src/pages/docs/server-multipart.mdx index a3cddab1698..a7474df6af6 100644 --- a/site/src/pages/docs/server-multipart.mdx +++ b/site/src/pages/docs/server-multipart.mdx @@ -140,7 +140,8 @@ public HttpResponse upload( The multipart files uploaded to an annotated service are stored into - and the files are not removed automatically. -You should explicitly **delete** the files after use. + and the files are removed depending on the +. If is set, you should explicitly +**delete** the files after use. From 4dc1fa2468dc2c70f09f85d59a34bbe06ec6ea50 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Thu, 2 May 2024 20:03:25 +0900 Subject: [PATCH 2/9] Remove `ON_JVM_SHUTDOWN` option --- .../armeria/internal/server/FileAggregatedMultipart.java | 3 --- .../linecorp/armeria/server/MultipartRemovalStrategy.java | 6 +----- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java index ce0a1952f5a..3b2fe18e5bb 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -137,9 +137,6 @@ private static Path createRemovableTempFile(Path targetDirectory, } }, blockingExecutorService); break; - case ON_JVM_SHUTDOWN: - tempFile.toFile().deleteOnExit(); - break; } return tempFile; } diff --git a/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java b/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java index b72c6a28f5b..937641dd486 100644 --- a/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java +++ b/core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java @@ -33,9 +33,5 @@ public enum MultipartRemovalStrategy { /** * Remove the temporary files after the response is fully sent. */ - ON_RESPONSE_COMPLETION, - /** - * Remove the temporary files when the JVM is shutting down. - */ - ON_JVM_SHUTDOWN + ON_RESPONSE_COMPLETION } From b75c256d59f02f23461e784882b01664336c39c8 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Thu, 2 May 2024 20:24:14 +0900 Subject: [PATCH 3/9] fix compile error --- .../common/SystemPropertyFlagsProvider.java | 2 -- .../server/MultipartTempFileRemovalTest.java | 19 ------------------- 2 files changed, 21 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index b961d670eb5..8ab72ce6037 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -486,8 +486,6 @@ public MultipartRemovalStrategy defaultMultipartRemovalStrategy() { return MultipartRemovalStrategy.NEVER; case "on_response_completion": return MultipartRemovalStrategy.ON_RESPONSE_COMPLETION; - case "on_jvm_shutdown": - return MultipartRemovalStrategy.ON_JVM_SHUTDOWN; default: throw new IllegalArgumentException( multipartRemovalStrategy + " isn't MultipartRemovalStrategy"); diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java index 5772a579c31..d9fb806b127 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java @@ -23,7 +23,6 @@ import java.nio.file.Files; import java.nio.file.Paths; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -58,10 +57,6 @@ protected void configure(ServerBuilder sb) throws Exception { .pathPrefix("/never") .multipartRemovalStrategy(MultipartRemovalStrategy.NEVER) .build(new TestMultipartService()); - sb.annotatedService() - .pathPrefix("/on_jvm_shutdown") - .multipartRemovalStrategy(MultipartRemovalStrategy.ON_JVM_SHUTDOWN) - .build(new TestMultipartService()); sb.annotatedService() .pathPrefix("/default") .build(new TestMultipartService()); @@ -85,20 +80,6 @@ void testRemovalStrategy(String type) throws Exception { } } - @Disabled("Enable this test to manually check if the temporary file is removed after the JVM shuts down.") - @CsvSource("/on_jvm_shutdown") - @ParameterizedTest - void testOnJvmShutDown(String type) throws Exception { - final BlockingWebClient client = server.blockingWebClient(); - final ContentDisposition contentDisposition = ContentDisposition.of("form-data", "file1", "file1.txt"); - final Multipart multipart = Multipart.of(BodyPart.of(contentDisposition, "file1 content")); - final AggregatedHttpResponse response = client.execute(multipart.toHttpRequest(type + "/upload")); - server.requestContextCaptor().take().log().whenComplete().join(); - final java.nio.file.Path path = Paths.get(response.contentUtf8()); - assertThat(Files.exists(path)).isTrue(); - logger.info("Please check if the temporary file is removed after the JVM shuts down: {}", path); - } - @Consumes(MediaTypeNames.MULTIPART_FORM_DATA) private static class TestMultipartService { @Blocking From feaeb6c1e4bfbac0c932b0d95d93dbfc608a76c0 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 7 May 2024 17:53:50 +0900 Subject: [PATCH 4/9] fix test failures and clean up --- .../com/linecorp/armeria/server/RoutersBenchmark.java | 6 +++++- .../server/AbstractAnnotatedServiceConfigSetters.java | 3 ++- .../server/ContextPathAnnotatedServiceConfigSetters.java | 6 +++--- .../java/com/linecorp/armeria/server/ServerBuilder.java | 1 - .../com/linecorp/armeria/server/VirtualHostBuilder.java | 8 +++++++- .../test/java/com/linecorp/armeria/common/FlagsTest.java | 6 ++++-- 6 files changed, 21 insertions(+), 9 deletions(-) diff --git a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java index 50c84e161d4..4c4c6096c15 100644 --- a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java +++ b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java @@ -67,19 +67,23 @@ public class RoutersBenchmark { SERVICE, defaultLogName, defaultServiceName, defaultServiceNaming, 0, 0, false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), SuccessFunction.always(), 0, multipartUploadsLocation, + MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), ctx -> RequestId.random(), serviceErrorHandler, NOOP_CONTEXT_HOOK), new ServiceConfig(route2, route2, SERVICE, defaultLogName, defaultServiceName, defaultServiceNaming, 0, 0, false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), SuccessFunction.always(), 0, multipartUploadsLocation, + MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), ctx -> RequestId.random(), serviceErrorHandler, NOOP_CONTEXT_HOOK)); FALLBACK_SERVICE = new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), SERVICE, defaultLogName, defaultServiceName, defaultServiceNaming, 0, 0, false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), SuccessFunction.always(), 0, - multipartUploadsLocation, CommonPools.workerGroup(), + multipartUploadsLocation, + MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, + CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), ctx -> RequestId.random(), serviceErrorHandler, NOOP_CONTEXT_HOOK); HOST = new VirtualHost( diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java index d800d2ad213..83a1634f2a1 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java @@ -284,7 +284,8 @@ public AbstractAnnotatedServiceConfigSetters multipartUploadsLocation(Path multi @UnstableApi @Override - public AbstractAnnotatedServiceConfigSetters multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + public AbstractAnnotatedServiceConfigSetters multipartRemovalStrategy( + MultipartRemovalStrategy removalStrategy) { defaultServiceConfigSetters.multipartRemovalStrategy(removalStrategy); return this; } diff --git a/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java index 4e8c8bccf64..50049098e66 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java @@ -51,9 +51,9 @@ public final class ContextPathAnnotatedServiceConfigSetters * Registers the given service to {@link ContextPathServicesBuilder} and returns the parent object. * * @param service annotated service object to handle incoming requests matching path prefix, which - * can be configured through {@link AnnotatedServiceBindingBuilder#pathPrefix(String)}. - * If path prefix is not set then this service is registered to handle requests matching - * {@code /} + * can be configured through {@link AnnotatedServiceBindingBuilder#pathPrefix(String)}. + * If path prefix is not set then this service is registered to handle requests matching + * {@code /} */ @Override public ContextPathServicesBuilder build(Object service) { diff --git a/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java b/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java index 45697b1341a..bbcf47f743e 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java @@ -958,7 +958,6 @@ public ServerBuilder multipartUploadsLocation(Path path) { * Sets the {@link MultipartRemovalStrategy} that determines when to remove temporary files created * for multipart requests. * If not set, {@link MultipartRemovalStrategy#ON_RESPONSE_COMPLETION} is used by default. - * */ @UnstableApi public ServerBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java index cf480e850c5..e2dfc602ff5 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java @@ -1193,9 +1193,15 @@ Path multipartUploadsLocation() { return multipartUploadsLocation; } + /** + * Sets the {@link MultipartRemovalStrategy} that determines when to remove temporary files created + * for multipart requests. + * If not set, {@link MultipartRemovalStrategy#ON_RESPONSE_COMPLETION} is used by default. + */ @UnstableApi - public void multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { + public VirtualHostBuilder multipartRemovalStrategy(MultipartRemovalStrategy removalStrategy) { multipartRemovalStrategy = requireNonNull(removalStrategy, "removalStrategy"); + return this; } /** diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index ff226943fe4..d7688b943ab 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -304,8 +304,10 @@ void testApiConsistencyBetweenFlagsAndFlagsProvider() { private ObjectAssert assertFlags(String flagsMethod) throws Throwable { final Lookup lookup = MethodHandles.publicLookup(); - final Class rtype = flagsClassLoader.loadClass(Flags.class.getMethod(flagsMethod).getReturnType() - .getCanonicalName()); + Class rtype = Flags.class.getMethod(flagsMethod).getReturnType(); + if (!rtype.isPrimitive()) { + rtype = flagsClassLoader.loadClass(rtype.getCanonicalName()); + } final MethodHandle method = lookup.findStatic(flags, flagsMethod, MethodType.methodType(rtype)); return assertThat(method.invoke()); From 005d22e8ff7852bdbca7e278c4a2382db8ac1bc0 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 8 May 2024 00:05:55 +0900 Subject: [PATCH 5/9] getter for VirtualHost --- .../linecorp/armeria/server/VirtualHost.java | 17 +++++++++++++++-- .../armeria/server/VirtualHostBuilder.java | 3 ++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java index 70e230cf4c2..aa47a6c6686 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java @@ -98,6 +98,7 @@ public final class VirtualHost { private final long requestAutoAbortDelayMillis; private final SuccessFunction successFunction; private final Path multipartUploadsLocation; + private final MultipartRemovalStrategy multipartRemovalStrategy; private final EventLoopGroup serviceWorkerGroup; private final List shutdownSupports; private final Function requestIdGenerator; @@ -117,6 +118,7 @@ public final class VirtualHost { long requestAutoAbortDelayMillis, SuccessFunction successFunction, Path multipartUploadsLocation, + MultipartRemovalStrategy multipartRemovalStrategy, EventLoopGroup serviceWorkerGroup, List shutdownSupports, Function requestIdGenerator) { @@ -141,6 +143,7 @@ public final class VirtualHost { this.requestAutoAbortDelayMillis = requestAutoAbortDelayMillis; this.successFunction = successFunction; this.multipartUploadsLocation = multipartUploadsLocation; + this.multipartRemovalStrategy = multipartRemovalStrategy; this.serviceWorkerGroup = serviceWorkerGroup; this.shutdownSupports = shutdownSupports; @SuppressWarnings("unchecked") @@ -168,7 +171,8 @@ VirtualHost withNewSslContext(SslContext sslContext) { host -> accessLogger, defaultServiceNaming, defaultLogName, requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, blockingTaskExecutor, requestAutoAbortDelayMillis, - successFunction, multipartUploadsLocation, serviceWorkerGroup, + successFunction, multipartUploadsLocation, multipartRemovalStrategy, + serviceWorkerGroup, shutdownSupports, requestIdGenerator); } @@ -544,6 +548,14 @@ public Path multipartUploadsLocation() { return multipartUploadsLocation; } + /** + * Returns the {@link MultipartRemovalStrategy} that specifies when to remove the temporary files created + * for multipart requests. + */ + public MultipartRemovalStrategy multipartRemovalStrategy() { + return multipartRemovalStrategy; + } + VirtualHost decorate(@Nullable Function decorator) { if (decorator == null) { return this; @@ -562,7 +574,8 @@ VirtualHost decorate(@Nullable Function accessLogger, defaultServiceNaming, defaultLogName, requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, blockingTaskExecutor, requestAutoAbortDelayMillis, successFunction, multipartUploadsLocation, - serviceWorkerGroup, shutdownSupports, requestIdGenerator); + multipartRemovalStrategy, serviceWorkerGroup, shutdownSupports, + requestIdGenerator); } @Override diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java index e2dfc602ff5..ec1f2b13a8a 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java @@ -1442,7 +1442,8 @@ VirtualHost build(VirtualHostBuilder template, DependencyInjector dependencyInje accessLoggerMapper, defaultServiceNaming, defaultLogName, requestTimeoutMillis, maxRequestLength, verboseResponses, accessLogWriter, blockingTaskExecutor, requestAutoAbortDelayMillis, successFunction, multipartUploadsLocation, - serviceWorkerGroup, builder.build(), requestIdGenerator); + multipartRemovalStrategy, serviceWorkerGroup, builder.build(), + requestIdGenerator); final Function decorator = getRouteDecoratingService(template, baseContextPath); From 25b2189e02dd56e658a28c5f7e4c276c9abfcb3a Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 8 May 2024 00:15:28 +0900 Subject: [PATCH 6/9] addres comments by @trustin --- site/src/pages/docs/server-multipart.mdx | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/site/src/pages/docs/server-multipart.mdx b/site/src/pages/docs/server-multipart.mdx index a7474df6af6..714f08982cf 100644 --- a/site/src/pages/docs/server-multipart.mdx +++ b/site/src/pages/docs/server-multipart.mdx @@ -139,9 +139,13 @@ public HttpResponse upload( -The multipart files uploaded to an annotated service are stored into - and the files are removed depending on the -. If is set, you should explicitly -**delete** the files after use. + and the files are removed as soon as the request is +handled completely by default, i.e. when the is complete. If you want to persist the +uploaded file, you can move the uploaded file to another folder or persistence layer before the deletion. + +Alternatively, you can disable the automatic deletion of the uploaded files by setting + to +, but please note that you must make +sure the uploaded files are deleted once they are not in use to avoid the excessive consumption of disk space. From 37d78bd31d2e208c5e47e8b89bb4985bf9cda1ea Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 8 May 2024 00:20:08 +0900 Subject: [PATCH 7/9] clean up --- .../main/java/com/linecorp/armeria/server/VirtualHost.java | 1 + .../test/java/com/linecorp/armeria/common/FlagsTest.java | 6 +----- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java b/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java index aa47a6c6686..fb56e7dba9f 100644 --- a/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java +++ b/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java @@ -552,6 +552,7 @@ public Path multipartUploadsLocation() { * Returns the {@link MultipartRemovalStrategy} that specifies when to remove the temporary files created * for multipart requests. */ + @UnstableApi public MultipartRemovalStrategy multipartRemovalStrategy() { return multipartRemovalStrategy; } diff --git a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java index d7688b943ab..2f780e70998 100644 --- a/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/FlagsTest.java @@ -342,11 +342,7 @@ public Class loadClass(String name) throws ClassNotFoundException { input.close(); final byte[] classData = buffer.toByteArray(); - final Class aClass = defineClass(name, classData, 0, classData.length); - if (name.contains("Multipart")) { - System.out.println("Loading class: " + name + ", class: " + aClass); - } - return aClass; + return defineClass(name, classData, 0, classData.length); } catch (IOException e) { Exceptions.throwUnsafely(e); } From f8cac3570bce0bf5c11e0ba3320ec8f861bcb081 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 8 May 2024 10:49:40 +0900 Subject: [PATCH 8/9] fix errors --- .../armeria/server/RoutersBenchmark.java | 53 ++++++++----------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java index 4c4c6096c15..35a218517f1 100644 --- a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java +++ b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java @@ -21,6 +21,7 @@ import java.nio.file.Path; import java.util.List; +import org.jetbrains.annotations.NotNull; import org.openjdk.jmh.annotations.Benchmark; import org.slf4j.helpers.NOPLogger; @@ -55,46 +56,36 @@ public class RoutersBenchmark { private static final RequestTarget METHOD1_REQ_TARGET = RequestTarget.forServer(METHOD1_HEADERS.path()); static { - final String defaultLogName = "log"; - final String defaultServiceName = null; - final ServiceNaming defaultServiceNaming = ServiceNaming.of("Service"); final Route route1 = Route.builder().exact("/grpc.package.Service/Method1").build(); final Route route2 = Route.builder().exact("/grpc.package.Service/Method2").build(); - final Path multipartUploadsLocation = Flags.defaultMultipartUploadsLocation(); - final ServiceErrorHandler serviceErrorHandler = ServerErrorHandler.ofDefault().asServiceErrorHandler(); - SERVICES = ImmutableList.of( - new ServiceConfig(route1, route1, - SERVICE, defaultLogName, defaultServiceName, defaultServiceNaming, 0, 0, - false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), 0, multipartUploadsLocation, - MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, - CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), - ctx -> RequestId.random(), serviceErrorHandler, NOOP_CONTEXT_HOOK), - new ServiceConfig(route2, route2, - SERVICE, defaultLogName, defaultServiceName, defaultServiceNaming, 0, 0, - false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), - SuccessFunction.always(), 0, multipartUploadsLocation, - MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, - CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), - ctx -> RequestId.random(), serviceErrorHandler, NOOP_CONTEXT_HOOK)); - FALLBACK_SERVICE = new ServiceConfig(Route.ofCatchAll(), Route.ofCatchAll(), SERVICE, - defaultLogName, defaultServiceName, - defaultServiceNaming, 0, 0, false, AccessLogWriter.disabled(), - CommonPools.blockingTaskExecutor(), SuccessFunction.always(), 0, - multipartUploadsLocation, - MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, - CommonPools.workerGroup(), - ImmutableList.of(), HttpHeaders.of(), ctx -> RequestId.random(), - serviceErrorHandler, NOOP_CONTEXT_HOOK); + SERVICES = ImmutableList.of(newServiceConfig(route1), newServiceConfig(route2)); + FALLBACK_SERVICE = newServiceConfig(Route.ofCatchAll()); HOST = new VirtualHost( "localhost", "localhost", 0, null, SERVICES, FALLBACK_SERVICE, RejectedRouteHandler.DISABLED, - unused -> NOPLogger.NOP_LOGGER, defaultServiceNaming, defaultLogName, 0, 0, false, + unused -> NOPLogger.NOP_LOGGER, FALLBACK_SERVICE.defaultServiceNaming(), + FALLBACK_SERVICE.defaultLogName(), 0, 0, false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), 0, SuccessFunction.ofDefault(), - multipartUploadsLocation, CommonPools.workerGroup(), ImmutableList.of(), + FALLBACK_SERVICE.multipartUploadsLocation(), MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, + CommonPools.workerGroup(), ImmutableList.of(), ctx -> RequestId.random()); ROUTER = Routers.ofVirtualHost(HOST, SERVICES, RejectedRouteHandler.DISABLED); } + private static @NotNull ServiceConfig newServiceConfig(Route route) { + final String defaultLogName = "log"; + final String defaultServiceName = null; + final ServiceNaming defaultServiceNaming = ServiceNaming.of("Service"); + final Path multipartUploadsLocation = Flags.defaultMultipartUploadsLocation(); + final ServiceErrorHandler serviceErrorHandler = ServerErrorHandler.ofDefault().asServiceErrorHandler(); + return new ServiceConfig(route, route, + SERVICE, defaultLogName, defaultServiceName, defaultServiceNaming, 0, 0, + false, AccessLogWriter.disabled(), CommonPools.blockingTaskExecutor(), + SuccessFunction.always(), 0, multipartUploadsLocation, + MultipartRemovalStrategy.ON_RESPONSE_COMPLETION, + CommonPools.workerGroup(), ImmutableList.of(), HttpHeaders.of(), + ctx -> RequestId.random(), serviceErrorHandler, NOOP_CONTEXT_HOOK); + } + @Benchmark public Routed exactMatch() { final RoutingContext ctx = DefaultRoutingContext.of(HOST, "localhost", METHOD1_REQ_TARGET, From 20c37c8b08dd61530d5a6bf7086ca87ef0eeebac Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Fri, 10 May 2024 17:27:20 +0900 Subject: [PATCH 9/9] addres comments and handle flaky tests --- .../jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java | 3 +-- .../linecorp/armeria/common/SystemPropertyFlagsProvider.java | 2 +- .../armeria/internal/server/MultipartTempFileRemovalTest.java | 3 ++- site/src/pages/docs/server-multipart.mdx | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java index 35a218517f1..9204452cc7c 100644 --- a/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java +++ b/benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java @@ -21,7 +21,6 @@ import java.nio.file.Path; import java.util.List; -import org.jetbrains.annotations.NotNull; import org.openjdk.jmh.annotations.Benchmark; import org.slf4j.helpers.NOPLogger; @@ -71,7 +70,7 @@ public class RoutersBenchmark { ROUTER = Routers.ofVirtualHost(HOST, SERVICES, RejectedRouteHandler.DISABLED); } - private static @NotNull ServiceConfig newServiceConfig(Route route) { + private static ServiceConfig newServiceConfig(Route route) { final String defaultLogName = "log"; final String defaultServiceName = null; final ServiceNaming defaultServiceNaming = ServiceNaming.of("Service"); diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index 8ab72ce6037..67253dee3f2 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -488,7 +488,7 @@ public MultipartRemovalStrategy defaultMultipartRemovalStrategy() { return MultipartRemovalStrategy.ON_RESPONSE_COMPLETION; default: throw new IllegalArgumentException( - multipartRemovalStrategy + " isn't MultipartRemovalStrategy"); + multipartRemovalStrategy + " isn't a MultipartRemovalStrategy"); } } diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java index d9fb806b127..f93ed32dfd8 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/server/MultipartTempFileRemovalTest.java @@ -17,6 +17,7 @@ package com.linecorp.armeria.internal.server; import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; import java.io.File; import java.io.IOException; @@ -76,7 +77,7 @@ void testRemovalStrategy(String type) throws Exception { assertThat(Files.exists(path)).isTrue(); Files.delete(path); } else { - assertThat(Files.exists(path)).isFalse(); + await().untilAsserted(() -> assertThat(Files.exists(path)).isFalse()); } } diff --git a/site/src/pages/docs/server-multipart.mdx b/site/src/pages/docs/server-multipart.mdx index 714f08982cf..9272c90c1e0 100644 --- a/site/src/pages/docs/server-multipart.mdx +++ b/site/src/pages/docs/server-multipart.mdx @@ -63,7 +63,7 @@ content-type:application/octet-stream ## Sending a multipart request The created in this way can be converted to an through - and transmitted to a server using a . + and transmitted to a server using a . ```java WebClient client = WebClient.of("https://armeria.dev");