Provide a way to automatically delete multipart temporary files #5653

ikhoon · 2024-04-30T06:13:49Z

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.

Server
  .builder()
  .multipartRemovalStrategy(MultipartRemovalStrategy.ON_RESPONSE_COMPLETION)

Fixes Automatically remove multipart temporary files #5652

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: line#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) ```

trustin · 2024-04-30T06:42:07Z

core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java

+                }, blockingExecutorService);
+                break;
+            case ON_JVM_SHUTDOWN:
+                tempFile.toFile().deleteOnExit();


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:

Introduce an additional flag purgeMultipartUploadsOnExit (default: true) that registers a shutdown hook that scans the directory and remove the items.

Remove ON_JVM_SHUTDOWN from (default)multipartRemovalStrategy.

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/

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.

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.

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

Sounds good to me.

github-actions · 2024-04-30T06:59:53Z

🔍 Build Scan® (commit: `ec4abe2`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/dja27ev646pb4
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/5nlo7kjvyinfw
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/62m6onluk2rg6
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/djzjtq5ebf7qa
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/jqxnkpfyj6xwe
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/p4qoy6apzonqq
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ilcigij56uggi
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/7ge2emykjysge

jrhee17 · 2024-05-07T03:02:37Z

The build seems to be failing. Is this PR ready for review?

ikhoon · 2024-05-07T08:56:34Z

Fixed all failures. This PR is now ready for review.

site/src/pages/docs/server-multipart.mdx

trustin · 2024-05-07T13:03:58Z

There's still one test failure left.

ikhoon · 2024-05-07T15:12:19Z

I wonder if it is useful to synchronize the getters of VirtualHost and ServiceConfig.
MultipartRemovalStrategy is used when there is a matching service. So ServiceConfig will always exist. VirtualHost.multipartRemovalStrategy() won't be used internally or by users.

Anyway, I fixed the failure.

trustin · 2024-05-08T05:42:51Z

ServiceConfig will always exist. VirtualHost.multipartRemovalStrategy() won't be used internally or by users.

A user could do this:

serverBuilder
  .withVirtualHost("foo.com")
    .multipartRemovalStrategy(NEVER)
    .service(...)
    .service(...);

Then virtual host level properties will be useful? If we have any service-level default, we should not do that, so that virtual host level (or default virtual host level if not set at virtual host level) is used when service level property is unset.

trustin

🚀🚀🚀

trustin · 2024-05-08T05:45:28Z

core/src/test/java/com/linecorp/armeria/server/ServiceNamingTest.java

-                                  routingCtx -> RequestId.of(1L),
-                                  ServerErrorHandler.ofDefault().asServiceErrorHandler(), NOOP_CONTEXT_HOOK);
+        final ServiceConfig config = newServiceConfig(HealthCheckService.builder().build(),
+                                                      ServiceNaming.fullTypeName());


👏👏👏

minwoox · 2024-05-08T08:45:46Z

Could you check the failure?

com.linecorp.armeria.internal.server.MultipartTempFileRemovalTest.[2] type=/default - 
org.opentest4j.AssertionFailedError: 
Expecting value to be false but was true
	at jdk.internal.reflect.GeneratedConstructorAccessor35.newInstance(Unknown Source)
	at java.base@17.0.10/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base@17.0.10/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at app//com.linecorp.armeria.internal.server.MultipartTempFileRemovalTest.testRemovalStrategy(MultipartTempFileRemovalTest.java:79)
	at java.base@17.0.10/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base@17.0.10/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	```

minwoox · 2024-05-08T08:47:20Z

This isn't relevant to your PR but could you also fix this line? MultiPart -> Multipart

armeria/site/src/pages/docs/server-multipart.mdx

Line 66 in 8bc6ca1

    
           <type://MultiPart#toHttpRequest(String)> and transmitted to a server using a <type://WebClient>.

benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java

jrhee17

Looks good to me once #5653 (comment) is handled 👍 Thanks @ikhoon ! 🙇 👍 🙇

core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java

ikhoon added new feature breaking change labels Apr 30, 2024

ikhoon added this to the 1.29.0 milestone Apr 30, 2024

ikhoon requested review from jrhee17, minwoox and trustin as code owners April 30, 2024 06:13

trustin reviewed Apr 30, 2024

View reviewed changes

ikhoon added 2 commits May 2, 2024 20:03

Remove ON_JVM_SHUTDOWN option

4dc1fa2

fix compile error

b75c256

fix test failures and clean up

feaeb6c

trustin reviewed May 7, 2024

View reviewed changes

site/src/pages/docs/server-multipart.mdx Outdated Show resolved Hide resolved

getter for VirtualHost

005d22e

ikhoon added 3 commits May 8, 2024 00:15

addres comments by @trustin

25b2189

clean up

37d78bd

fix errors

f8cac35

trustin approved these changes May 8, 2024

View reviewed changes

minwoox reviewed May 8, 2024

View reviewed changes

benchmarks/jmh/src/jmh/java/com/linecorp/armeria/server/RoutersBenchmark.java Outdated Show resolved Hide resolved

jrhee17 approved these changes May 10, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java Outdated Show resolved Hide resolved

ikhoon added 2 commits May 10, 2024 17:27

addres comments and handle flaky tests

20c37c8

Merge branch 'main' into delete-multipart-temp-files

ec4abe2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to automatically delete multipart temporary files #5653

Provide a way to automatically delete multipart temporary files #5653

ikhoon commented Apr 30, 2024 •

edited

trustin Apr 30, 2024

ikhoon Apr 30, 2024 •

edited

jrhee17 May 2, 2024 •

edited

ikhoon May 2, 2024

trustin May 2, 2024

github-actions bot commented Apr 30, 2024 •

edited

jrhee17 commented May 7, 2024

ikhoon commented May 7, 2024

trustin commented May 7, 2024

ikhoon commented May 7, 2024

trustin commented May 8, 2024

trustin left a comment

trustin May 8, 2024

minwoox commented May 8, 2024

minwoox commented May 8, 2024

jrhee17 left a comment

Provide a way to automatically delete multipart temporary files #5653

Are you sure you want to change the base?

Provide a way to automatically delete multipart temporary files #5653

Conversation

ikhoon commented Apr 30, 2024 • edited

trustin Apr 30, 2024

Choose a reason for hiding this comment

ikhoon Apr 30, 2024 • edited

Choose a reason for hiding this comment

jrhee17 May 2, 2024 • edited

Choose a reason for hiding this comment

ikhoon May 2, 2024

Choose a reason for hiding this comment

trustin May 2, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 30, 2024 • edited

🔍 Build Scan® (commit: ec4abe2)

jrhee17 commented May 7, 2024

ikhoon commented May 7, 2024

trustin commented May 7, 2024

ikhoon commented May 7, 2024

trustin commented May 8, 2024

trustin left a comment

Choose a reason for hiding this comment

trustin May 8, 2024

Choose a reason for hiding this comment

minwoox commented May 8, 2024

minwoox commented May 8, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon commented Apr 30, 2024 •

edited

ikhoon Apr 30, 2024 •

edited

jrhee17 May 2, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

🔍 Build Scan® (commit: `ec4abe2`)