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

@RouteFilter stopped working with WebSocket requests Quarkus 3.2.0.Final #34908

Closed
AlekseyMay opened this issue Jul 21, 2023 · 19 comments · Fixed by #35188
Closed

@RouteFilter stopped working with WebSocket requests Quarkus 3.2.0.Final #34908

AlekseyMay opened this issue Jul 21, 2023 · 19 comments · Fixed by #35188
Labels
area/vertx kind/bug Something isn't working
Milestone

Comments

@AlekseyMay
Copy link

AlekseyMay commented Jul 21, 2023

Describe the bug

We were migrating from 2.16.7 to 3.2.0.Final

And we have WebSocket interceptor implemented with @RouteFilter

It looks like this:

@RouteFilter
 void authFilter(RoutingContext rc) {
    /// logic to search for websocket header etc
}

We use it to intercept GQL Subscriptions and its initial websocket request.
After migration to 3.2.0.Final it stopped intercepting WebSocket for some reason.

Expected behavior

Intercept all requests as it was in 2.16.7

Actual behavior

Not intercepting websocket initial request

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

17

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.0.Final

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

gradle 8.1.1

Additional information

No response

@AlekseyMay AlekseyMay added the kind/bug Something isn't working label Jul 21, 2023
@geoand
Copy link
Contributor

geoand commented Jul 21, 2023

cc @mkouba

@AlekseyMay
Copy link
Author

@mkouba
Any help or comments pls, maybe I am doing smth wrong with @RouteFilter in quarkus 3+?
Really need this functionality...

@mkouba
Copy link
Contributor

mkouba commented Aug 1, 2023

We use it to intercept GQL Subscriptions and its initial websocket request.
After migration to 3.2.0.Final it stopped intercepting WebSocket for some reason.

I'm not aware of any change in the implementation of @RouteFilter.

Maybe it's related to some change in the smallrye-graphql?

CC @phillip-kruger @jmartisk

@jmartisk
Copy link
Contributor

jmartisk commented Aug 1, 2023

This doesn't look dependent on GraphQL, nor am I aware of any changes in this area. Perhaps a reproducer project would help?

@AlekseyMay
Copy link
Author

AlekseyMay commented Aug 1, 2023

Reproducer.zip

@jmartisk @mkouba

Thank you for the response. Here is the reproducer.

To reproduce you can do the following:

  1. start the app
  2. try to call query
query {
  sayHello
 }
  1. You'll see that the request is intercepted and the headers are printed in the console.
  2. Then try to initiate websocket connection. It can be manually called with 'ws://localhost:8085/graphql' from Postman or it can be done from dev gql ui by calling subscribe .
  3. If you call it manually then you will see nothing in console, if you subscribed from ui you'll see the log from controller that is printed on subscription command. And no log from interceptor.

After all this the version of Quarkus can be changed to 2.16.9.Final in pom.xml.
You can try the same steps, and you should see the interception of websocket initiation and logs printed from interceptor on websocket initiation request.

This is very strange, maybe I am doing smth wrong...

@uPagge
Copy link

uPagge commented Aug 3, 2023

Hello everyone. I have the same problem :(

@mkouba
Copy link
Contributor

mkouba commented Aug 3, 2023

Hm, the SmallRyeGraphQLOverWebSocketHandler which handles the HTTP UPGRADE header is registered with priority Integer.MIN_VALUE and does not call the next route so I would say that the filter route should not be called. But the same priority is used in 2.16 🤷.

@phillip-kruger
Copy link
Member

Is it not working in any v3.x ? Or just 3.2.0 ? Because then it might be Dev UI ? Also check Dev Mode vs Prod mode ? That should rule out (or in) Dev UI

@AlekseyMay
Copy link
Author

AlekseyMay commented Aug 3, 2023

@phillip-kruger @mkouba

I have tested several versions from 3 up to 3.2.2 and the problem always appears.

I've recorded small video showing the problem using the Reproducer that is attached, maybe it can help to understand the problem. First I try with 3.2.0 Final then same steps with 2.16.9 Final. And the request is only intercepted in 2.16.9 Final.
https://drive.google.com/file/d/1Mi5HCaVy2fWL5CP5b52nypP0Gp-k8Jaq/view

@phillip-kruger
Copy link
Member

@AlekseyMay can you test in non dev mode ? So build the jar and run with java -jar ?

@AlekseyMay
Copy link
Author

@phillip-kruger

For me the result is the the same. Intercepted with 2.16.9 and not with 3.2.0. I ve done the same steps as on video.

image

image

@phillip-kruger
Copy link
Member

Thanks ! That kind of rule out Dev UI. @jmartisk @mkouba are you going to look at this ? I can't look at this right away.

@mkouba
Copy link
Contributor

mkouba commented Aug 3, 2023

I've recorded small video showing the problem using the Reproducer that is attached, maybe it can help to understand the problem.

I do understand the problem and I'm able to reproduce it. But I'm still not convinced it should work. Like I mentioned in #34908 (comment).

Thanks ! That kind of rule out Dev UI. @jmartisk @mkouba are you going to look at this ? I can't look at this right away.

I'm trying to debug the reproducer. With 2.16 the com.example.Interceptor.filter(RoutingContext) is executed before SmallRyeGraphQLOverWebSocketHandler even though it has order=-10 and SmallRyeGraphQLOverWebSocketHandler has order=-2147483648. Maybe the behavior changed between Vert.x 4.3.7 and 4.4.4? IIUIC it was fixed.

@mkouba
Copy link
Contributor

mkouba commented Aug 3, 2023

Ok, I found it. It worked by coincidence because HttpRootPathBuildItem.Builder.orderedRoute() was never used before quarkus 3.0, i.e. the value -1 was always used and therefore the com.example.Interceptor#filter() which has the default priority for filters: -10, was executed before the SmallRyeGraphQLOverWebSocketHandler. We fixed this bug in #32008. So now the ordering is correct but since the SmallRyeGraphQLOverWebSocketHandler is registered with order=-2147483648 it's always executed before the Interceptor#filter().

@phillip-kruger I believe that SmallRyeGraphQLOverWebSocketHandler should use a different value so that it's possible to register a filter that can be executed before this handler. WDYT?

@phillip-kruger
Copy link
Member

Nice ! Thanks @mkouba . Yes for sure. I recall some internal ranges somewhere.... It's probably on Integer.MAX_VALUE at the moment, we should defiantly change that. @mkouba do you want to do the fix ? Else @jmartisk can add it to our todo list :)

@mkouba
Copy link
Contributor

mkouba commented Aug 3, 2023

Nice ! Thanks @mkouba . Yes for sure. I recall some internal ranges somewhere.... It's probably on Integer.MAX_VALUE at the moment, we should defiantly change that. @mkouba do you want to do the fix ? Else @jmartisk can add it to our todo list :)

I can send a PR shortly. I'm not aware of any "internal ranges" but using a fixed value such as -1000 should be enough.

@phillip-kruger
Copy link
Member

Perfect ! Thanks.

mkouba added a commit to mkouba/quarkus that referenced this issue Aug 3, 2023
- so that it's possible to register a route/filter that is executed
before this handler
- fixes quarkusio#34908
@mkouba
Copy link
Contributor

mkouba commented Aug 3, 2023

@AlekseyMay So once the #35188 is merged you can use @RouteFilter(10_001) (or any value > 10000).

mkouba added a commit to mkouba/quarkus that referenced this issue Aug 4, 2023
- so that it's possible to register a route/filter that is executed
before this handler
- fixes quarkusio#34908
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Aug 4, 2023
@AlekseyMay
Copy link
Author

@mkouba @phillip-kruger
Thanks a lot for the help

@gsmet gsmet modified the milestones: 3.3.0.CR1, 3.2.4.Final Aug 9, 2023
gsmet pushed a commit to gsmet/quarkus that referenced this issue Aug 9, 2023
- so that it's possible to register a route/filter that is executed
before this handler
- fixes quarkusio#34908

(cherry picked from commit 2be7e95)
benkard pushed a commit to benkard/mulkcms2 that referenced this issue Aug 29, 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.214.0` -> `^0.215.0`](https://renovatebot.com/diffs/npm/flow-bin/0.214.0/0.215.1) |
| [org.liquibase:liquibase-maven-plugin](http://www.liquibase.org/liquibase-maven-plugin) ([source](https://github.com/liquibase/liquibase)) | build | patch | `4.23.0` -> `4.23.1` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | minor | `3.2.3.Final` -> `3.3.0` |
| [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | minor | `3.2.3.Final` -> `3.3.0` |
| [org.apache.maven.plugins:maven-enforcer-plugin](https://maven.apache.org/enforcer/) | build | minor | `3.3.0` -> `3.4.0` |

---

### Release Notes

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

### [`v0.215.1`](flow/flow-bin@a92ce80...cbb038f)

[Compare Source](flow/flow-bin@a92ce80...cbb038f)

### [`v0.215.0`](flow/flow-bin@ca11e28...a92ce80)

[Compare Source](flow/flow-bin@ca11e28...a92ce80)

</details>

<details>
<summary>liquibase/liquibase</summary>

### [`v4.23.1`](https://github.com/liquibase/liquibase/blob/HEAD/changelog.txt#Liquibase-4231-is-a-patch-release)

[Compare Source](liquibase/liquibase@v4.23.0...v4.23.1)

</details>

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

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

[Compare Source](quarkusio/quarkus@3.2.4.Final...3.3.0)

##### Complete changelog

-   [#&#8203;35350](quarkusio/quarkus#35350) - Fix package type system property clearing
-   [#&#8203;35348](quarkusio/quarkus#35348) - quarkus-maven-plugin runs native building even if the profile is commented out
-   [#&#8203;35343](quarkusio/quarkus#35343) - ArC: fix StackOverflowError in AutoAddScopeBuildItem
-   [#&#8203;35319](quarkusio/quarkus#35319) - Register arrays of Hibernate ORM's JDBC basic types for reflection
-   [#&#8203;35315](quarkusio/quarkus#35315) - Fix Datasource timing issues with Liquibase / Flyway and OpenTelemetry
-   [#&#8203;35314](quarkusio/quarkus#35314) - Regression in 3.3.0.CR1: Synthetic bean instance for io.opentelemetry.api.OpenTelemetry not initialized yet
-   [#&#8203;35312](quarkusio/quarkus#35312) - Updates Infinispan to 14.0.13.Final
-   [#&#8203;35308](quarkusio/quarkus#35308) - Lock jib execution to avoid OverlappingFileLockException in parallel builds
-   [#&#8203;35305](quarkusio/quarkus#35305) - Fix the titles of the tables in RESTEasy Reactive doc
-   [#&#8203;35302](quarkusio/quarkus#35302) - Docs: Mention wilcard support in resteasy reactive XML serialisation exclude classes configuration
-   [#&#8203;35301](quarkusio/quarkus#35301) - Fix potential NPE in quarkus-csrf-reactive when no MediaType is found
-   [#&#8203;35299](quarkusio/quarkus#35299) - Output build graph using `quarkus.builder.graph-output` property
-   [#&#8203;35285](quarkusio/quarkus#35285) - NullPointerException during http post request when quarkus-csrf-reactive extension is added to a project
-   [#&#8203;35283](quarkusio/quarkus#35283) - Upgrade proto-google-common-protos to 2.23.0
-   [#&#8203;35282](quarkusio/quarkus#35282) - Avoid keeping references to BytecodeRecorderImpl
-   [#&#8203;35276](quarkusio/quarkus#35276) - Reinstate DevModeTestUtil to avoid breaking other projects that depend on it
-   [#&#8203;35273](quarkusio/quarkus#35273) - Fix small typo in comment
-   [#&#8203;35263](quarkusio/quarkus#35263) - Stop the recovery service while ArC is still around
-   [#&#8203;35245](quarkusio/quarkus#35245) - Add missing info to init Jobs
-   [#&#8203;35244](quarkusio/quarkus#35244) - Init Jobs are missing ServiceAccount and Image Pull Secrets
-   [#&#8203;35240](quarkusio/quarkus#35240) - Update SmallRye Health to 4.0.4
-   [#&#8203;34071](quarkusio/quarkus#34071) - 3.1.1 Final - java.lang.IllegalArgumentException: Class java.util.UUID\[] is instantiated reflectively but was never registered
-   [#&#8203;32800](quarkusio/quarkus#32800) - Duplicated checks in health check response
-   [#&#8203;11903](quarkusio/quarkus#11903) - Gradle multimodule project + quarkus-container-image-jib: OverlappingFileLockException

### [`v3.2.4.Final`](https://github.com/quarkusio/quarkus/releases/tag/3.2.4.Final)

[Compare Source](quarkusio/quarkus@3.2.3.Final...3.2.4.Final)

##### Complete changelog

-   [#&#8203;35300](quarkusio/quarkus#35300) - Fix `jandex-gradle-plugin-version` in CDI Reference
-   [#&#8203;35296](quarkusio/quarkus#35296) - Upgrade H2 to 2.2.220
-   [#&#8203;35258](quarkusio/quarkus#35258) - CDI Reference 1.1 has incomplete information for gradle
-   [#&#8203;35255](quarkusio/quarkus#35255) - Quartz: QuarkusMSSQLDelegate should extends MSSQLDelegate
-   [#&#8203;35211](quarkusio/quarkus#35211) - Document Maven config options that may be relevant when running tests
-   [#&#8203;35203](quarkusio/quarkus#35203) - Pass Maven user settings when initializing artifact resolver
-   [#&#8203;35193](quarkusio/quarkus#35193) - OpenTelemetry service name should have higher priority than app name when no resource attributes are set
-   [#&#8203;35189](quarkusio/quarkus#35189) - Quarkus CLI fixes
-   [#&#8203;35188](quarkusio/quarkus#35188) - SmallRyeGraphQLOverWebSocketHandler: use order value > Integer.MIN_VALUE
-   [#&#8203;35181](quarkusio/quarkus#35181) - REST Data with Panache should not produce links when hal is disabled
-   [#&#8203;35174](quarkusio/quarkus#35174) - Ensure the narayana-jta extension fully shuts down the recovery manager
-   [#&#8203;35172](quarkusio/quarkus#35172) - Kafka Streams: restore the feature name at Quarkus startup
-   [#&#8203;35171](quarkusio/quarkus#35171) - kafka-streams: feature not listed on startup
-   [#&#8203;35165](quarkusio/quarkus#35165) - Propagate all user methods in REST Data with Panache
-   [#&#8203;35160](quarkusio/quarkus#35160) - Properly use internal links to point to other guides
-   [#&#8203;35140](quarkusio/quarkus#35140) - ArC: fix deadlock when calling guarded methods on the same thread
-   [#&#8203;35136](quarkusio/quarkus#35136) - Deadlock while calling write-locked method from read-locked method
-   [#&#8203;34908](quarkusio/quarkus#34908) - `@RouteFilter` stopped working with WebSocket requests Quarkus 3.2.0.Final
-   [#&#8203;34875](quarkusio/quarkus#34875) - Quarkus build does not work since 3.2.0 with teamcity/plexus launcher
-   [#&#8203;34713](quarkusio/quarkus#34713) - Option to track build configuration for changes between builds
-   [#&#8203;34576](quarkusio/quarkus#34576) - Live reload stopped working on 3.2 when using XA transactions
-   [#&#8203;34514](quarkusio/quarkus#34514) - Support `@WithUnnamedKey` in documentation
-   [#&#8203;34065](quarkusio/quarkus#34065) - Add support for project Java version update based on extensions
-   [#&#8203;33317](quarkusio/quarkus#33317) - OpenTelemetry SDK autoconfiguration ignores OTEL service name in favor of Quarkus app name
-   [#&#8203;15461](quarkusio/quarkus#15461) - Quarkus tests fails mTLS authentication against internal Maven repository

</details>

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

### [`v3.3.0`](quarkusio/quarkus-platform@3.2.4.Final...3.3.0)

[Compare Source](quarkusio/quarkus-platform@3.2.4.Final...3.3.0)

### [`v3.2.4.Final`](quarkusio/quarkus-platform@3.2.3.Final...3.2.4.Final)

[Compare Source](quarkusio/quarkus-platform@3.2.3.Final...3.2.4.Final)

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

---

 - [x] <!-- 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-->
benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this issue Sep 21, 2023
…s-googlecloud-jsonlogging!17)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io.quarkus:quarkus-extension-processor](https://github.com/quarkusio/quarkus) |  | minor | `3.2.3.Final` -> `3.3.0` |
| [io.quarkus:quarkus-extension-maven-plugin](https://github.com/quarkusio/quarkus) | build | minor | `3.2.3.Final` -> `3.3.0` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | minor | `3.2.3.Final` -> `3.3.0` |
| [io.quarkus:quarkus-bom](https://github.com/quarkusio/quarkus) | import | minor | `3.2.3.Final` -> `3.3.0` |

---

### Release Notes

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

### [`v3.3.0`](quarkusio/quarkus@3.2.4.Final...3.3.0)

[Compare Source](quarkusio/quarkus@3.2.4.Final...3.3.0)

### [`v3.2.4.Final`](https://github.com/quarkusio/quarkus/releases/tag/3.2.4.Final)

[Compare Source](quarkusio/quarkus@3.2.3.Final...3.2.4.Final)

##### Complete changelog

-   [#&#8203;35300](quarkusio/quarkus#35300) - Fix `jandex-gradle-plugin-version` in CDI Reference
-   [#&#8203;35296](quarkusio/quarkus#35296) - Upgrade H2 to 2.2.220
-   [#&#8203;35258](quarkusio/quarkus#35258) - CDI Reference 1.1 has incomplete information for gradle
-   [#&#8203;35255](quarkusio/quarkus#35255) - Quartz: QuarkusMSSQLDelegate should extends MSSQLDelegate
-   [#&#8203;35211](quarkusio/quarkus#35211) - Document Maven config options that may be relevant when running tests
-   [#&#8203;35203](quarkusio/quarkus#35203) - Pass Maven user settings when initializing artifact resolver
-   [#&#8203;35193](quarkusio/quarkus#35193) - OpenTelemetry service name should have higher priority than app name when no resource attributes are set
-   [#&#8203;35189](quarkusio/quarkus#35189) - Quarkus CLI fixes
-   [#&#8203;35188](quarkusio/quarkus#35188) - SmallRyeGraphQLOverWebSocketHandler: use order value > Integer.MIN_VALUE
-   [#&#8203;35181](quarkusio/quarkus#35181) - REST Data with Panache should not produce links when hal is disabled
-   [#&#8203;35174](quarkusio/quarkus#35174) - Ensure the narayana-jta extension fully shuts down the recovery manager
-   [#&#8203;35172](quarkusio/quarkus#35172) - Kafka Streams: restore the feature name at Quarkus startup
-   [#&#8203;35171](quarkusio/quarkus#35171) - kafka-streams: feature not listed on startup
-   [#&#8203;35165](quarkusio/quarkus#35165) - Propagate all user methods in REST Data with Panache
-   [#&#8203;35160](quarkusio/quarkus#35160) - Properly use internal links to point to other guides
-   [#&#8203;35140](quarkusio/quarkus#35140) - ArC: fix deadlock when calling guarded methods on the same thread
-   [#&#8203;35136](quarkusio/quarkus#35136) - Deadlock while calling write-locked method from read-locked method
-   [#&#8203;34908](quarkusio/quarkus#34908) - `@RouteFilter` stopped working with WebSocket requests Quarkus 3.2.0.Final
-   [#&#8203;34875](quarkusio/quarkus#34875) - Quarkus build does not work since 3.2.0 with teamcity/plexus launcher
-   [#&#8203;34713](quarkusio/quarkus#34713) - Option to track build configuration for changes between builds
-   [#&#8203;34576](quarkusio/quarkus#34576) - Live reload stopped working on 3.2 when using XA transactions
-   [#&#8203;34514](quarkusio/quarkus#34514) - Support `@WithUnnamedKey` in documentation
-   [#&#8203;34065](quarkusio/quarkus#34065) - Add support for project Java version update based on extensions
-   [#&#8203;33317](quarkusio/quarkus#33317) - OpenTelemetry SDK autoconfiguration ignores OTEL service name in favor of Quarkus app name
-   [#&#8203;15461](quarkusio/quarkus#15461) - Quarkus tests fails mTLS authentication against internal Maven repository

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

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- 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-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertx kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants