reactor-netty icon indicating copy to clipboard operation
reactor-netty copied to clipboard

Published 20 hours ago • verified publisher icon reactor

Reintegrate multipart into netty5 branch

Open pderop opened this issue 3 years ago • 12 comments

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.

pderop avatar Jun 26 '22 23:06 pderop

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

violetagg avatar Jun 27 '22 07:06 violetagg

@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 avatar Jun 27 '22 10:06 pderop

@violetagg,

I think this PR is ready for a review. The other Netty MP PR (https://github.com/netty-contrib/codec-multipart/pull/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.

pderop avatar Jul 07 '22 13:07 pderop

@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 avatar Jul 07 '22 13:07 violetagg

I added status blocked

violetagg avatar Jul 07 '22 13:07 violetagg

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 a001ed3e6c21e666a7d54ed3fb4f6ecd42c59a8f):

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 a001ed3e6c21e666a7d54ed3fb4f6ecd42c59a8f):

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

pderop avatar Aug 23 '22 23:08 pderop

@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):

https://github.com/netty-contrib/codec-multipart/pull/12

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

thanks

pderop avatar Aug 24 '22 19:08 pderop

@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

violetagg avatar Aug 25 '22 06:08 violetagg

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.

pderop avatar Aug 25 '22 11:08 pderop

Rebase in order to pick up the last changes in netty5 branch (expecially PR #2449)

pderop avatar Aug 25 '22 12:08 pderop

fixed up previous commit, where a missing import was remaining.

pderop avatar Aug 25 '22 12:08 pderop

rebased on netty5, in order to fix a conflict in .github/workflows/check_transport.yml, which is now using all netty-contribs from their main branches.

pderop avatar Aug 31 '22 13:08 pderop

rebased on latest netty5 branch in order to pickup #2484

pderop avatar Sep 30 '22 16:09 pderop

Two PRs are not yet merged in the netty-contrib MP project, so I temporarily changed the workflows/check_transport.yml in order to build netty-contrib/codec-multipart from my own repo (this is temporary, and the check_transport.yml will have to be overridden later, with the latest one from netty5 branch).

So, I rebased on top of latest netty5 branch, essentially to pick up #2484. Got few problems while rebasing, but now tests are OK.

What still need to be done is to finalize the declarations of excluded methods in japicmp, in build.gradle.

pderop avatar Sep 30 '22 18:09 pderop

rebased on top of netty5 branch, to pick up (#2551).

pderop avatar Oct 21 '22 08:10 pderop

forced push in order to revert the two previous commits which were wrong (b7c6fd3 and 02244f8

pderop avatar Oct 24 '22 16:10 pderop

forced push with a fixup commit for old wrong commit ad7621c ([temporary] build codec-multipart before reactor-netty)

pderop avatar Oct 24 '22 17:10 pderop

@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.

pderop avatar Oct 24 '22 18:10 pderop

@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 avatar Oct 24 '22 18:10 violetagg

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

pderop avatar Oct 26 '22 18:10 pderop

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

pderop avatar Oct 27 '22 14:10 pderop

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 avatar Nov 02 '22 10:11 pderop

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 avatar Nov 02 '22 17:11 pderop