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

notify observer to complete async stream #5008

Merged
merged 7 commits into from
May 8, 2022
Merged

notify observer to complete async stream #5008

merged 7 commits into from
May 8, 2022

Conversation

CoreDX9
Copy link
Contributor

@CoreDX9 CoreDX9 commented Apr 27, 2022

notify observer to complete async stream.

  • If convert IObservable{T} to IAsyncEnumerable and use await foreach to enumerate stream, but observers never call OnCompleted, the thread will be permanently blocked.

see also #5006 .

@PascalSenn
Copy link
Member

Very cool, thanks for the contribution :)

Can you add one test case in the integration tests to test this behaviour?

You can add a test case to
https://github.com/ChilliCream/hotchocolate/blob/main/src/StrawberryShake/CodeGeneration/test/CodeGeneration.CSharp.Tests/Integration/TestGeneration.cs
that will generate the client.

And then just type out a test case like
https://github.com/ChilliCream/hotchocolate/blob/main/src/StrawberryShake/CodeGeneration/test/CodeGeneration.CSharp.Tests/Integration/StarWarsGetFriendsTest.cs

@CoreDX9
Copy link
Contributor Author

CoreDX9 commented Apr 27, 2022

No problem, but I need some time.

@CoreDX9
Copy link
Contributor Author

CoreDX9 commented Apr 27, 2022

In addition, this pull request can only let the client detect the subscription that ends normally. If the socket is closed due to server crash or network error, the client cannot receive the message of completing the subscription, so this notification will not be triggered. If you want to complete the subscription in this case, you may need to modify the interface definition. Because my interim solution needs to access non-public members through reflection to trigger the notification of subscription completion. If you can accept it, I am willing to initiate another pull request to solve this problem.

@CoreDX9
Copy link
Contributor Author

CoreDX9 commented Apr 28, 2022

@PascalSenn integration test has added.

Copy link
Member

@michaelstaib michaelstaib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me... thanks for your help!

@michaelstaib
Copy link
Member

@PascalSenn I will patch this into 12.9 so let's keep it open for now... I will merge it on the weekend.

@michaelstaib michaelstaib added the 🎬 ready Ready to merge label Apr 28, 2022
@michaelstaib
Copy link
Member

OK ... we now have a green main ... so lets see if this build is green.

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sonarcloud
Copy link

sonarcloud bot commented May 4, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants