ffang commented on PR #3660: URL: https://github.com/apache/cxf/pull/3660#issuecomment-6606730350
> Hi @ffang , Good catch thanks! . Sorry my fault, I apologize. I had run only http transport tests cause my pc is slow as a turtle and I have some conflict to generate all the resources . > > Now I applied the change as U describe and added the http2 GET test that without your fix fail also with: > > > JettyClientServerHttp2Test.testGetWithChunkingHttp2:79 » Processing java.io.IOException: IOException invoking https://localhost:39501/http2/web/bookstore/pluto: java.io.UncheckedIOException: java.io.IOException: Pipe not connected > > Now it runs ok along with other tests :) ! > > Only one doubt : > > > One problem though: when contentLen == 0 (GET/DELETE) > > Here U said also DELETE method, but I see: > > > if (KNOWN_HTTP_VERBS_WITH_NO_CONTENT.contains(httpRequestMethod) > > || PropertyUtils.isTrue(outMessage.get(Headers.EMPTY_REQUEST_PROPERTY))) { > > contentLen = 0; > > } > > protected static final Set KNOWN_HTTP_VERBS_WITH_NO_CONTENT = > > new HashSet<>(Arrays.asList(new String[]{"GET", "HEAD", "OPTIONS", "TRACE"})); > > So either we are sure that DELETE meets 'PropertyUtils.isTrue(outMessage.get(Headers.EMPTY_REQUEST_PROPERTY))' or we need to be sure that who use DELETE with nobody "doesn't subscribe"/ "set manually the contentLen to 0" . > > Let me know what U think... and if the new commits are ok for U. > > Thanks again, Valentino Porta Hi @vp340, Thanks for the quick update. The change and the new test look good: `JettyClientServerHttp2Test` and `JettyClientServerHttp2cTest` pass, and if I revert only the conduit line, `testGetWithChunkingHttp2` fails with `Pipe not connected`. About DELETE, you're right that "GET/DELETE" was loose on my side: DELETE isn't in `KNOWN_HTTP_VERBS_WITH_NO_CONTENT`. It gets `contentLen = 0` only through `org.apache.cxf.empty.request`, but the JAX-RS client sets that for any request without an entity (`AbstractClient.setEmptyRequestPropertyIfNeeded`), so `WebClient.delete()` without a body had the same problem as GET. Any other bodiless request keeps `contentLen = -1`, so `pout` is created and the pipe is connected. Since the new condition `contentLen != 0` is the same one that decides whether `pout` is created, the publisher gets `pin` exactly when the pipe is connected, whatever the HTTP method. Nothing needs to be set manually. Minor, not blocking: - The commits can be squashed on merge, and the title could be `CXF-9267: ...`（So the PR here can be linked to the jira ticket automatically ）. +1 from me. Best Regards Freeman -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
