Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide a way to automatically delete multipart temporary files #5653

Merged
merged 10 commits into from
May 30, 2024
Merged

Provide a way to automatically delete multipart temporary files #5653

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
330402c
Provide a way to automatically delete multipart temporary files
ikhoon Apr 30, 2024
4dc1fa2
Remove `ON_JVM_SHUTDOWN` option
ikhoon May 2, 2024
b75c256
fix compile error
ikhoon May 2, 2024
feaeb6c
fix test failures and clean up
ikhoon May 7, 2024
005d22e
getter for VirtualHost
ikhoon May 7, 2024
25b2189
addres comments by @trustin
ikhoon May 7, 2024
37d78bd
clean up
ikhoon May 7, 2024
f8cac35
fix errors
ikhoon May 8, 2024
20c37c8
addres comments and handle flaky tests
ikhoon May 10, 2024
ec4abe2
Merge branch 'main' into delete-multipart-temp-files
ikhoon May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -460,6 +461,11 @@ public Path defaultMultipartUploadsLocation() {
File.separatorChar + "multipart-uploads");
}

@Override
public MultipartRemovalStrategy defaultMultipartRemovalStrategy() {
return MultipartRemovalStrategy.ON_RESPONSE_COMPLETION;
}

@Override
public Sampler<? super RequestContext> requestContextLeakDetectionSampler() {
return Sampler.never();
Expand Down
13 changes: 13 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/Flags.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<? super RequestContext> REQUEST_CONTEXT_LEAK_DETECTION_SAMPLER =
getValue(FlagsProvider::requestContextLeakDetectionSampler, "requestContextLeakDetectionSampler");

Expand Down Expand Up @@ -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.
*
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
21 changes: 21 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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");
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
}
}

@Override
public Sampler<? super RequestContext> requestContextLeakDetectionSampler() {
final String spec = getNormalized("requestContextLeakDetectionSampler");
Expand Down
37 changes: 33 additions & 4 deletions core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String, String> params;
private final ListMultimap<String, MultipartFile> files;

Expand Down Expand Up @@ -72,7 +77,7 @@ public static CompletableFuture<FileAggregatedMultipart> 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()));
});
Expand Down Expand Up @@ -102,19 +107,43 @@ public static CompletableFuture<FileAggregatedMultipart> aggregateMultipart(Serv
}

private static CompletableFuture<Path> 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();
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it result in a kind of resource leak, since the JVM needs to keep the list of files to remove on exit?

What do you think about this:

  1. Introduce an additional flag purgeMultipartUploadsOnExit (default: true) that registers a shutdown hook that scans the directory and remove the items.
  2. Remove ON_JVM_SHUTDOWN from (default)multipartRemovalStrategy.

Copy link
Contributor Author

@ikhoon ikhoon Apr 30, 2024 •
edited

Choose a reason for hiding this comment

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

Introduce an additional flag purgeMultipartUploadsOnExit (default: true)

I think purgeMultipartUploadsOnExit and MultipartRemovalStrategy are mutually exclusive. If MultipartRemovalStrategy.ON_RESPONSE_COMPLETION is set, purgeMultipartUploadsOnExit means nothing. If some users want to keep the temporary files for n days as a backup even after the server restarts, they have to set both MultipartRemovalStrategy.NEVER and purgeMultipartUploadsOnExit=false.

that registers a shutdown hook that scans the directory and remove the items.

How about moving multipart files to a special directory such as temporary if ON_JVM_SHUTDOWN is set? The shutdown hook will delete all temporary directories under multipartUploadsLocations.

- service1-upload/
  - incomplete/
  - running/ <- for ON_RESPONSE_COMPLETION
  - complete/ <- for NEVER
  - temporary/ <- for ON_JVM_SHUTDOWN
- service2-upload/
  - incomplete/
  - running/
  - temporary/

Copy link
Contributor

@jrhee17 jrhee17 May 2, 2024 •
edited

Choose a reason for hiding this comment

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

I'm +1 for @ikhoon 's idea to at least group the user-facing APIs in one place. I do think we will need to add a shutdown hook somewhere if ON_JVM_SHUTDOWN is selected though.

Eventually, I imagine an API like the following (if very advanced features are needed)

val mps = MultipartRemovalStrategy.
  // builder methods
  .onJvmShutdown()
  .onResponseComplete()
  .never()
  // maybe advanced
  .builderForScheduledDeletion().poll(1 day).olderThan(7 days)
ServerBuilder.multipartRemovalStrategy(mps)

How about moving multipart files to a special directory such as temporary if ON_JVM_SHUTDOWN is set?

I'm not sure I understood the need to separate the directory between complete and temporary.

Also, would it make sense to somehow embed the server id for each multipart file in case multiple servers are running in the same jvm? This way, we can still delete files that are for a specific jvm and the flag ON_JVM_SHUTDOWN makes more sense.

e.g. <pid>-<server id>- as the prefix for the temporary files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if ON_JVM_SHUTDOWN is a really useful option, it is worth implementing the complicated functions. For now, I would like to delete ON_JVM_SHUTDOWN from the PR and make it unsupported. If users have requests for it, we would like to consider them.

Copy link
Member

Choose a reason for hiding this comment

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

For now, I would like to delete ON_JVM_SHUTDOWN from the PR and make it unsupported.

Sounds good to me.

break;
}
return tempFile;
}

private static CompletableFuture<Path> resolveTmpFile(Path directory,
String filename,
ExecutorService blockingExecutorService) {
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/AbstractAnnotatedServiceConfigSetters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/AnnotatedServiceBindingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<? super RoutingContext, ? extends RequestId> requestIdGenerator) {
Expand Down
12 changes: 9 additions & 3 deletions core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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<? super RoutingContext, ? extends RequestId> requestIdGenerator) {
Expand Down
11 changes: 7 additions & 4 deletions core/src/main/java/com/linecorp/armeria/server/ContextPathServiceBindingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 11 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/DefaultServiceConfigSetters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down
41 changes: 41 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/MultipartRemovalStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
*
* <p><strong>Warning:</strong> 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
}
14 changes: 14 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/ServiceBindingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down