Reintegrate multipart into netty5 branch #2327

pderop · 2022-06-26T23:09:24Z

Related to #2221

This PR is an attempt to reintegrate the Multipart feature into netty5 branch using new netty-contrib multipart codec.
The old multipart code that has bee reintegrated comes from the latest 1.0.x branch.

violetagg · 2022-06-27T07:21:08Z

@pderop Once #2328 is merged, please rebase and modify the CI build so that the PR in Netty Contrib is built before Reactor Netty

reactor-netty-core/build.gradle

pderop · 2022-06-27T10:14:23Z

@violetagg , I have rebased to get the #2328 and the CI build has been modified in order to build the Netty Contrib MP before RN.

pderop · 2022-07-07T13:24:05Z

@violetagg,

I think this PR is ready for a review.
The other Netty MP PR (netty-contrib/codec-multipart#1) status is also in request-for-review, so I hope Chris or Norman will have some time to take a look into it.

In the mean time, if you want to start reviewing this PR, that would be great.
thanks.

violetagg · 2022-07-07T13:40:36Z

@pderop Let's wait for the dependent PR in Netty Contrib and when we know that the API will not be changed, I'll proceed with reviewing this one.

violetagg · 2022-07-07T13:41:37Z

I added status blocked

pderop · 2022-08-23T23:40:16Z

Rebased the PR on top of latest netty5 branch.

To summarize, this PR has merged the following files from latest 1.0.x branch (SHA a001ed3):

reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClient.java
reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientFinalizer.java
reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java
reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerFormDecoderProvider.java
reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java
reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerRequest.java
Reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java
Reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientWithTomcatTest.java
Reactor-netty-http/src/test/java/reactor/netty/http/server/HttpServerTests.java

And the following files have been re-added and merged from latest 1.0.x branch (SHA a001ed3):

reactor-netty-examples/src/main/java/reactor/netty/examples/documentation/http/server/multipart/Application.java
reactor-netty-examples/src/main/java/reactor/netty/examples/documentation/http/server/multipart/custom/Application.java
reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientForm.java
Reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientFormEncoder.java
Reactor-netty-http/src/test/java/reactor/netty/http/server/HttpServerFormDecoderProviderTests.java
Reactor-netty-http/src/test/java/reactor/netty/http/server/HttpServerPostFormTests.java

reactor-netty5-http/build.gradle

violetagg

This is an initial review ... I need additional time to check the functional correctness ...

Please address the additional warnings reported by the build. Thanks!

reactor-netty5-http/build.gradle

reactor-netty5-http/src/main/java/reactor/netty5/http/client/HttpClientFinalizer.java

reactor-netty5-http/src/main/java/reactor/netty5/http/server/HttpServerFormDecoderProvider.java

reactor-netty5-http/src/test/java/reactor/netty5/http/server/HttpServerTests.java

reactor-netty5-http/src/main/java/reactor/netty5/http/client/HttpClientOperations.java

pderop · 2022-08-24T19:54:56Z

@violetagg ,

in order to fix the additional compilation warnings, I had to do a fix in the netty-contrib, so I have created this PR (but I have not merged it):

netty-contrib/codec-multipart#12

Do you think I can go ahead and merge it or should I ask Chris to review ?

thanks

violetagg · 2022-08-25T06:32:21Z

@violetagg ,

in order to fix the additional compilation warnings, I had to do a fix in the netty-contrib, so I have created this PR (but I have not merged it):

netty-contrib/codec-multipart#12

Do you think I can go ahead and merge it or should I ask Chris to review ?

It is better to receive feedback at least from someone

reactor-netty5-http/src/test/java/reactor/netty5/http/server/HttpServerTests.java

pderop · 2022-08-25T11:17:32Z

The netty-contrib PR is now used from the httpcontent-used-as-raw-type, because the PR is not yet merged.

the CI still reports some errors, I'm now looking into them.

violetagg · 2022-10-24T18:47:38Z

@violetagg ,

apart from CodeQL, I think this PR is ready for a second review. I will see tomorrow what to do for CodeQL. thank you.

No need to check CodeQL for the time being. Once the third party project is released CodeQL should be working again.

violetagg

@pderop I need some clarifications before continuing with the rest of the review.

reactor-netty5-http/src/main/java/reactor/netty5/http/client/HttpClientFinalizer.java

reactor-netty5-http/src/main/java/reactor/netty5/http/client/HttpClientFormEncoder.java

reactor-netty5-http/src/main/java/reactor/netty5/http/client/HttpClientOperations.java

reactor-netty5-http/src/main/java/reactor/netty5/http/server/HttpServerFormDecoderProvider.java

…0.Alpha1-SNAPSHOT

…ged GH workflow to use PR#12 from httpcontent-used-as-raw-type repo

…the same as before

… is disabled.

pderop · 2022-10-26T18:04:58Z

rebased in order to be on top of latest netty5 branch.

…le uploads with streams

…osing it in destroy method.

pderop · 2022-10-27T14:28:18Z

@violetagg , please take a look to previous commits, thank you.

violetagg

@pderop Once we have a release for the third party project, please update the version and remove the CI changes and then we can merge this PR.

…multipart.

pderop · 2022-11-02T10:50:53Z

all tests are now failing, but it sounds like it's not related to this PR, indeed, we have this test which is now failing, but it also fails when using latest netty5 branch:

TcpClientTests > tcpClientHandlesLineFeedDataElasticPool() FAILED
    java.util.concurrent.CancellationException
        at io.netty5.util.concurrent.DefaultPromise.cancel(...)(Unknown Source)

more specifically, here is the root error:

java.util.concurrent.CancellationException
	at io.netty5.util.concurrent.DefaultPromise.cancel(...)(Unknown Source)
	Suppressed: java.lang.Exception: #block terminated with an error
		at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:139)
		at reactor.core.publisher.Mono.block(Mono.java:1733)
		at reactor.netty5.tcp.TcpClientTests.tcpClientHandlesLineFeedData(TcpClientTests.java:339)
		at reactor.netty5.tcp.TcpClientTests.tcpClientHandlesLineFeedDataElasticPool(TcpClientTests.java:308)
		at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
		at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
		at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
		at java.base/java.lang.reflect.Method.invoke(Method.java:568)
		at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
		at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
		at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
		at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
		at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
		at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
		at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
		at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
		at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
		at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
		at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
		at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
		at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
		at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
		at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:217)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:213)
		at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:138)
		at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:68)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
		at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
		at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
		at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
		at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
		at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
		at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
		at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
		at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
		at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
		at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
		at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
		at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
		at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
		at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
		at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
		at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
		at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
		at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
		at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
		at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:53)
		at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
		at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
		at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
		at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
		at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
		at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
		at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
		at java.base/java.lang.reflect.Method.invoke(Method.java:568)
		at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
		at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
		at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
		at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
		at jdk.proxy2/jdk.proxy2.$Proxy5.stop(Unknown Source)
		at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
		at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
		at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
		at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
		at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
		at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
		at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
		at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
		at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

I'll try to figure out before merging this PR.

pderop · 2022-11-02T17:45:31Z

so, there are still many errors, but I don't think they are caused by this PR.
So in order to go ahead, let's squash & merge this old overdue PR.

pderop changed the title ~~[WIP] Reintegrate multipart into nett5 branch~~ [WIP] Reintegrate multipart into netty5 branch Jun 26, 2022

pderop self-assigned this Jun 26, 2022

pderop added the type/enhancement A general enhancement label Jun 26, 2022

pderop added this to the 2.0.0-M1 milestone Jun 26, 2022

violetagg reviewed Jun 27, 2022

View reviewed changes

reactor-netty-core/build.gradle Outdated Show resolved Hide resolved

pderop changed the title ~~[WIP] Reintegrate multipart into netty5 branch~~ Reintegrate multipart into netty5 branch Jul 7, 2022

pderop marked this pull request as ready for review July 7, 2022 13:21

violetagg added the status/blocked An issue that's blocked on an external project change label Jul 7, 2022

pderop modified the milestones: 2.0.0-M1, 2.0.0-M2 Jul 11, 2022

violetagg linked an issue Aug 2, 2022 that may be closed by this pull request

Adapt Reactor Netty Multipart functionality to Netty 5 changes #2221

Closed

Rattenkrieg mentioned this pull request Aug 3, 2022

Multipart upload from Publisher<ByteBuf> #2418

Open

pderop force-pushed the netty5-reintegrate-multipart branch from e8c8934 to f1f736f Compare August 23, 2022 23:33

pderop marked this pull request as draft August 24, 2022 06:26

pderop mentioned this pull request Aug 24, 2022

Netty5.x Multipart Codec for Netty Contrib netty-contrib/codec-multipart#1

Merged

pderop marked this pull request as ready for review August 24, 2022 09:30

pderop requested a review from violetagg August 24, 2022 09:30

violetagg removed the status/blocked An issue that's blocked on an external project change label Aug 24, 2022

violetagg reviewed Aug 24, 2022

View reviewed changes

reactor-netty5-http/build.gradle Outdated Show resolved Hide resolved

violetagg requested changes Aug 24, 2022

View reviewed changes

violetagg reviewed Aug 25, 2022

View reviewed changes

reactor-netty5-http/src/test/java/reactor/netty5/http/server/HttpServerTests.java Outdated Show resolved Hide resolved

violetagg requested changes Oct 25, 2022

View reviewed changes

pderop added 15 commits October 26, 2022 20:02

Reintegrate multipart into netty5 branch

287b11a

Applied feedback: we now depend on netty-contrib MP with version 5.0.…

8a91664

…0.Alpha1-SNAPSHOT

Applied feedback

463cdab

polish: moved HttpServerRequest into server section

0c20614

Applied feedback: fixed japicmp, HttpServerTests instanceof, and chan…

272be5f

…ged GH workflow to use PR#12 from httpcontent-used-as-raw-type repo

We can now use all netty-contribs from their main branches

6f5030b

Adapt to new Netty Buffer package name

0c70c86

[temporary] build codec-multipart before reactor-netty

98e608a

finalized previous rebase

5ba166f

Adapt to new netty contrib multipart API change

0ece17e

Fixed memory leak in HttpClientFormEncoder

60f9804

removed HttpClient#sendForm from japicmp since method signatures are …

b4b46b7

…the same as before

Added separator line before static import

354f648

Let's create the Mono if it is needed. This happens only when chunked…

e176492

… is disabled.

Corrected previous commit

0d5704e

pderop force-pushed the netty5-reintegrate-multipart branch from 51a59af to 0d5704e Compare October 26, 2022 18:04

pderop added 3 commits October 26, 2022 20:08

Applied reactor#2556 to fix memory leak in HttpClient#sendForm for fi…

0f62d3c

…le uploads with streams

No need to check if current partial http data is accessible before cl…

618035a

…osing it in destroy method.

Copy partial data before inserting it to currentCompletedHttpData list

868e1bd

violetagg approved these changes Oct 28, 2022

View reviewed changes

Updated CI and top level build.gradle in order to use released codec-…

5b26c47

…multipart.

pderop modified the milestones: 2.0.x Backlog, 2.0.0-M3 Nov 2, 2022

pderop merged commit 996d8a8 into reactor:netty5 Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintegrate multipart into netty5 branch #2327

Reintegrate multipart into netty5 branch #2327

pderop commented Jun 26, 2022 •

edited

Loading

violetagg commented Jun 27, 2022

pderop commented Jun 27, 2022 •

edited

Loading

pderop commented Jul 7, 2022

violetagg commented Jul 7, 2022

violetagg commented Jul 7, 2022

pderop commented Aug 23, 2022

violetagg left a comment •

edited

Loading

pderop commented Aug 24, 2022 •

edited

Loading

violetagg commented Aug 25, 2022

pderop commented Aug 25, 2022

violetagg commented Oct 24, 2022

violetagg left a comment

pderop commented Oct 26, 2022

pderop commented Oct 27, 2022

violetagg left a comment

pderop commented Nov 2, 2022

pderop commented Nov 2, 2022

Reintegrate multipart into netty5 branch #2327

Reintegrate multipart into netty5 branch #2327

Conversation

pderop commented Jun 26, 2022 • edited Loading

violetagg commented Jun 27, 2022

pderop commented Jun 27, 2022 • edited Loading

pderop commented Jul 7, 2022

violetagg commented Jul 7, 2022

violetagg commented Jul 7, 2022

pderop commented Aug 23, 2022

violetagg left a comment • edited Loading

Choose a reason for hiding this comment

pderop commented Aug 24, 2022 • edited Loading

violetagg commented Aug 25, 2022

pderop commented Aug 25, 2022

violetagg commented Oct 24, 2022

violetagg left a comment

Choose a reason for hiding this comment

pderop commented Oct 26, 2022

pderop commented Oct 27, 2022

violetagg left a comment

Choose a reason for hiding this comment

pderop commented Nov 2, 2022

pderop commented Nov 2, 2022

pderop commented Jun 26, 2022 •

edited

Loading

pderop commented Jun 27, 2022 •

edited

Loading

violetagg left a comment •

edited

Loading

pderop commented Aug 24, 2022 •

edited

Loading