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.

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

Rest client call hangs when receiving an invalid chunked response and does not release resources (e.g. Bulkhead semaphore) #36257

Closed
tmonney opened this issue Oct 3, 2023 · 5 comments · Fixed by #36351
Assignees
Labels
area/rest-client kind/bug Something isn't working
Milestone

Comments

@tmonney
Copy link

tmonney commented Oct 3, 2023

Describe the bug

When a RestClient resource receives an invalid chunked response (e.g. with missing chunk length), the response is never passed to the caller: the CompletionStage or Uni is never completed, a plain response is simply blocked indefinitely.
This happens even if the remote server closes the connection.

The behavior can be simulated with Wiremock's Fault.MALFORMED_RESPONSE_CHUNK, which writes the Transfer-Encoding: chunked header, writes some garbage (and no chunk length), then closes the connection.

When using a simple Vert.x WebClient or HttpClient, the request does not hang and the caller gets an HttpClosedException, as I would expect.

I also noticed that if a @Bulkhead is applied to the resource, each such invalid response acquires a permit from the bulkhead's semaphore and never releases it, potentially exhausting the available permits and making all requests fail until the application is restarted.

Expected behavior

An invalid chunked response should not hang the client and resources should be properly released when the connection is closed.

Actual behavior

The client call never completes and resources are not released.

How to Reproduce?

See https://github.com/tmonney/mp-rest-client-malformed-chunk

Output of uname -a or ver

Linux WL-HHDYFL3 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27 02:56:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

java version "17.0.8" 2023-07-18 LTS
Java(TM) SE Runtime Environment (build 17.0.8+9-LTS-211)
Java HotSpot(TM) 64-Bit Server VM (build 17.0.8+9-LTS-211, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.4.1

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.4 (dfbb324ad4a7c8fb0bf182e6d91b0ae20e3d2dd9)

Additional information

No response

@tmonney tmonney added the kind/bug Something isn't working label Oct 3, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 3, 2023

/cc @cescoffier (rest-client), @geoand (rest-client)

@geoand geoand self-assigned this Oct 9, 2023
@geoand
Copy link
Contributor

geoand commented Oct 9, 2023

Thanks for the reproducer!

I'll have a look ASAP

@geoand
Copy link
Contributor

geoand commented Oct 9, 2023

Turns out this was a classic case of forgetting to do proper error handling :(

#36351 fixes the issue

@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Oct 9, 2023
geoand added a commit that referenced this issue Oct 9, 2023
Properly handle invalid response body errors in Reactive REST Client
@tmonney
Copy link
Author

tmonney commented Oct 9, 2023

Great, thanks a lot for the quick fix.

@geoand
Copy link
Contributor

geoand commented Oct 9, 2023

🙏

@gsmet gsmet modified the milestones: 3.5.0.CR1, 3.4.3 Oct 11, 2023
gsmet pushed a commit to gsmet/quarkus that referenced this issue Oct 11, 2023
@aloubyansky aloubyansky modified the milestones: 3.4.3, 3.2.8.Final Oct 25, 2023
aloubyansky pushed a commit to aloubyansky/quarkus that referenced this issue Oct 25, 2023
benkard pushed a commit to benkard/mulkcms2 that referenced this issue Nov 12, 2023
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.217.0` -> `^0.219.0`](https://renovatebot.com/diffs/npm/flow-bin/0.217.2/0.219.0) |
| [io.hypersistence:hypersistence-utils-hibernate-62](https://github.com/vladmihalcea/hypersistence-utils) | compile | minor | `3.5.3` -> `3.6.0` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.4.2` -> `3.4.3` |
| [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `3.4.2` -> `3.4.3` |

---

### Release Notes

<details>
<summary>flowtype/flow-bin</summary>

### [`v0.219.0`](flow/flow-bin@1268ec5...c184c5d)

[Compare Source](flow/flow-bin@1268ec5...c184c5d)

### [`v0.218.1`](flow/flow-bin@12afce4...1268ec5)

[Compare Source](flow/flow-bin@12afce4...1268ec5)

### [`v0.218.0`](flow/flow-bin@dc93913...12afce4)

[Compare Source](flow/flow-bin@dc93913...12afce4)

</details>

<details>
<summary>vladmihalcea/hypersistence-utils</summary>

### [`v3.6.0`](https://github.com/vladmihalcea/hypersistence-utils/blob/HEAD/changelog.txt#Version-360---October-12-2023)

\================================================================================

Implement QueryStackTraceLogger using StackWalker [#&#8203;659](vladmihalcea/hypersistence-utils#659)

Add JFR based query logger [#&#8203;658](vladmihalcea/hypersistence-utils#658)

Adds support for using MonetaryAmount in [@&#8203;ElementCollection](https://github.com/ElementCollection) [#&#8203;652](vladmihalcea/hypersistence-utils#652)

</details>

<details>
<summary>quarkusio/quarkus</summary>

### [`v3.4.3`](https://github.com/quarkusio/quarkus/releases/tag/3.4.3)

[Compare Source](quarkusio/quarkus@3.4.2...3.4.3)

##### Complete changelog

-   [#&#8203;36408](quarkusio/quarkus#36408) - Ensure that SSE builder works in native
-   [#&#8203;36404](quarkusio/quarkus#36404) - Do not exclude properties from recording that are available in sources that should always be included
-   [#&#8203;36403](quarkusio/quarkus#36403) - IBM Db2 - Register resource bundle classes for reflection
-   [#&#8203;36402](quarkusio/quarkus#36402) - Native Picocli build breaks SSE client
-   [#&#8203;36399](quarkusio/quarkus#36399) - quarkus-jdbc-db2: resource bundle missing
-   [#&#8203;36377](quarkusio/quarkus#36377) - Allow `@ClientHeaderParam` to override User-Agent
-   [#&#8203;36371](quarkusio/quarkus#36371) - Fix issue in Java migration in dev-mode
-   [#&#8203;36351](quarkusio/quarkus#36351) - Properly handle invalid response body errors in Reactive REST Client
-   [#&#8203;36329](quarkusio/quarkus#36329) - Custom User-Agent header ignored
-   [#&#8203;36326](quarkusio/quarkus#36326) - Cannot load fixed or default YAML configuration when running native build
-   [#&#8203;36302](quarkusio/quarkus#36302) - Fix headers and preambles in all guides and reintroduce some keywords
-   [#&#8203;36299](quarkusio/quarkus#36299) - quarkus-flyway: Java-Migration does not get picked up on live reload
-   [#&#8203;36290](quarkusio/quarkus#36290) - Fixed URL for configuring JSON support
-   [#&#8203;36257](quarkusio/quarkus#36257) - Rest client call hangs when receiving an invalid chunked response and does not release resources (e.g. Bulkhead semaphore)
-   [#&#8203;36147](quarkusio/quarkus#36147) - Bump org.eclipse.parsson:parsson from 1.1.2 to 1.1.4
-   [#&#8203;36096](quarkusio/quarkus#36096) - Build cache - Use notCacheableBecause instead of storeEnabled
-   [#&#8203;35929](quarkusio/quarkus#35929) - Do not store build cache for core extensions having config
-   [#&#8203;35927](quarkusio/quarkus#35927) - Build cache - Core extensions containing config shouldn't be cached
-   [#&#8203;35769](quarkusio/quarkus#35769) - Default response content type using GraphQL spec

</details>

<details>
<summary>quarkusio/quarkus-platform</summary>

### [`v3.4.3`](quarkusio/quarkus-platform@3.4.2...3.4.3)

[Compare Source](quarkusio/quarkus-platform@3.4.2...3.4.3)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest-client kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants