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

Provide service provider to subscription execution #691

Merged
merged 4 commits into from
Dec 30, 2021

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Dec 29, 2021

Fixes #690

The new IGraphQLBuilder extension methods rely on the service provider to be passed to the document executer. Without it, many of the extension methods will not function as intended.

This PR creates a scope for the duration of the execution. The scope and acquired services are disposed immediately after execution completes. In this way, there are no unintended extremely-long lifetimes of scoped objects. Also, since execution only occurs at the start of a subscription, there is no need to hold the scope indefinitely. Services that provide the IObservable instances must still be singletons, as is required now, because if they were scoped then (a) they would not share the event stream data across multiple instances (b) they would be disposed when the execution is disposed.

@Shane32 Shane32 added this to the v5.2 milestone Dec 29, 2021
@Shane32 Shane32 requested a review from sungam3r December 29, 2021 16:12
@Shane32 Shane32 self-assigned this Dec 29, 2021
@github-actions github-actions bot added the test label Dec 29, 2021
@Shane32 Shane32 mentioned this pull request Dec 29, 2021
3 tasks
@sungam3r
Copy link
Member

Nice fix.

@sungam3r
Copy link
Member

изображение

😠
изображение

@sungam3r sungam3r mentioned this pull request Dec 30, 2021
@sungam3r sungam3r added bugfix Pull request that fixes a bug and removed test labels Dec 30, 2021
@github-actions github-actions bot added the test label Dec 30, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #691 (d1fa727) into master (37fd20d) will increase coverage by 0.04%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
+ Coverage   50.93%   50.97%   +0.04%     
==========================================
  Files          68       68              
  Lines        1818     1838      +20     
  Branches      183      184       +1     
==========================================
+ Hits          926      937      +11     
- Misses        838      847       +9     
  Partials       54       54              
Impacted Files Coverage Δ
...criptions.WebSockets/WebSocketConnectionFactory.cs 0.00% <0.00%> (ø)
....Subscriptions.Abstractions/SubscriptionManager.cs 70.00% <100.00%> (+3.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37fd20d...d1fa727. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscription fail on latest master
3 participants