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

Use actix-ws for juniper_actix subscriptions #1197

Merged
merged 8 commits into from
Oct 24, 2023
Merged

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Oct 24, 2023

Follows #1191, #1186

Motivation

See #1186 (comment):

  1. Reimplement juniper_actix::subscritions via actorless actix-ws crate instead of the current actix-web-actors::ws one. The whole thing with actors cooperates very badly with juniper_graphql_ws state machine, introducing too excessive polling, which leads to panics and abnormal WebSocket closures instead of normal ones. It also gives a feeling of fighting with actors, trying to pair them with Stream/Sink, instead of easing it. So, the actorless actix-ws crate should go just fine, as we have in the juniper_warp crate.

Also, WebSocket connections are not closed normally in juniper_actix, due to panics:
Screenshot 2023-10-24 at 15 58 48
Screenshot 2023-10-24 at 15 59 41

Solution

Get rid of actix-web-actors::ws and use actix-ws crate instead.

Now, WebSocket connections are closing as expected:
Screenshot 2023-10-24 at 16 02 24
Screenshot 2023-10-24 at 16 02 44

Additionally

  • Fixes some issues with the graphql-transport-ws implementation, regarding panicking for edge cases in polling.
  • Adds juniper::http integration tests for graphql-transport-ws as well.

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems area::subscriptions Related to GraphQL subscriptions lib::actix Related to `actix` crate integration labels Oct 24, 2023
@tyranron tyranron added this to the 0.16.0 milestone Oct 24, 2023
@tyranron tyranron self-assigned this Oct 24, 2023
@tyranron tyranron marked this pull request as ready for review October 24, 2023 15:45
@tyranron tyranron enabled auto-merge (squash) October 24, 2023 17:19
@tyranron tyranron merged commit d11e351 into master Oct 24, 2023
166 checks passed
@tyranron tyranron deleted the rework-actix-ws branch October 24, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area::subscriptions Related to GraphQL subscriptions enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems lib::actix Related to `actix` crate integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant