ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.26k stars 745 forks source link

StrawberryShake subscription returns before ready - timing issue #4824

Open benbierens opened 2 years ago

benbierens commented 2 years ago

Is there an existing issue for this?

Describe the bug

HT_subscription

Context: Topic: Automated API/integration tests. Project: dotgraphee Issue: StrawberryShake's subscription .Watch().Subscribe() seem to return before the system is ready to receive subscriptions. (Cleanup: .Subscribe() returns an IDisposable which is disposed by the test fixture TearDown. We didn't forget it. 😉 ) Workaround: Thread.Sleep(1000); after subscription is started. 😢

Steps to reproduce

  1. Set up a HotChocolate endpoint with subscriptions.
  2. Set up a StrawberryShake client to subscribe and trigger that subscription with a mutation.
  3. Create a test that calls the mutation immediately after starting the subscription, see image above. (dotgraphee is building a branch 'feature/strawberry-shake-client' that does step 1, 2 and 3.)
  4. Test may pass randomly or when run in isolation. Create several or run in loop to reproduce.

Relevant log output

No response

Additional Context?

No response

Product

Strawberry Shake

Version

12.6.2

michaelstaib commented 2 years ago

Do you have a small repro?

Watch essentiall returns an observable and subscribe registeres a delegate that receives updates from the entity store. You will get controll immediately. But may be I am misunderstanding what you expectation is in this case ...

If you use System.Reactive you can also convert the observable into an async stream and interate over it which will give you a blocking call until something arrives.

kolpav commented 2 years ago

I had flaky test similar to what you describe but maybe my setup was wrong. To do subscribe->mutation->wait for subscription response->assert kind of test I was doing something like this.

var timeout = TimeSpan.FromSeconds(5);
var cancellationTokenSource = new CancellationTokenSource(timeout);
var value = 0;
using var _ = Client.OnSaleBid.Watch(saleId).Subscribe(response =>
{
  value = response.Data.OnValue.Value;
  cancellationTokenSource.Cancel();
});

await Client.Foo.ExecuteAsync(CancellationToken.Token);

await Task.Delay(-1, cancellationTokenSource.Token).ContinueWith(_ => { }, CancellationToken.None);
value.Should().BePositive();

It usually happened in CI where multiple tests run in parallel.

michaelstaib commented 2 years ago

Delays are no good to deal with these kinds of tests.

kolpav commented 2 years ago

@michaelstaib Same error happened when I rewrote my test using System.Reactive I guess there is race condition somewhere or something in my fixture setup which is more likely because I had to copy paste code from HC to make strawberry shake to use test server's web socket client. When I was asking for help on slack you said "Wait for the subscription stitching pr" which I assume would make it easier to do. Is the PR already part of some release? I could then update my fixture to see if the error still happens.