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

Fix issue of potentially dropped messages in subscription #279

Closed
wants to merge 3 commits into from

Conversation

jinhong-
Copy link

Fixed and refactored code to utilize async APIs, without using Task.Wait(), which potentially creates deadlocks. I have tested this to work in my project

@pekkah
Copy link
Collaborator

pekkah commented Oct 25, 2019

I think you still need to handle the case of SendAsync returning false. Trying again would be good or allowing user to specify what to do.

@jinhong-
Copy link
Author

jinhong- commented Nov 7, 2019

@pekkah Hmmm i am not sure if SendAsync actually returns a boolean as seen below:

Task SendAsync(OperationMessage message);

I understand that underlying it is using ActionBlock.SendAsync, which provides a boolean return value. However upon reading the documentations and examples, there are plenty of results that point to the fact that it should not return false unless the block completes or fails
Question then now is if we want allow user to specify what to do, what would be the best way to do this? Here's what i am thinking:

On the event of a failure (SendAsync returns false):

  1. Log failure
  2. Terminate Subscription

Retries could be possible, however that would require passing in some sort of configuration from the root 3 levels into the Subscriptions class if we do not want to hard code the retry count

@jinhong-
Copy link
Author

jinhong- commented Nov 7, 2019

I made a small tweak to help with handling errors and terminating the subscription

@jinhong-
Copy link
Author

jinhong- commented Feb 6, 2020

@pekkah Are you able to take a look at the change and see if there are any more changes required?

@jinhong- jinhong- requested a review from pekkah February 6, 2020 02:25
@sungam3r sungam3r added needs investigation The described problem requires additional time to study BREAKING Breaking changes in either public API or runtime behavior labels Feb 22, 2020
@Shane32 Shane32 changed the base branch from develop to master October 26, 2020 23:32
@Shane32
Copy link
Member

Shane32 commented Jul 11, 2022

These changes should not be necessary in version 7 of GraphQL.Server

@Shane32 Shane32 closed this Jul 11, 2022
@sungam3r sungam3r removed the needs investigation The described problem requires additional time to study label Jul 11, 2022
@Shane32 Shane32 mentioned this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING Breaking changes in either public API or runtime behavior subscriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants