Skip to content

Refactor LongPolling in Java to avoid stackoverflow #33564

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

Merged
merged 2 commits into from
Jun 17, 2021

Conversation

BrennanConroy
Copy link
Member

Fixes #29477

Switch from recursion to subject.onNext() for re-polling.

@BrennanConroy BrennanConroy added area-signalr Includes: SignalR clients and servers feature-client-java Related to the SignalR Java client labels Jun 15, 2021
@BrennanConroy BrennanConroy requested a review from halter73 June 15, 2021 21:45
} else {
logger.debug("Poll timed out, reissuing.");
}
}
return poll(url);
receiveLoopSubject.onNext(url);
Copy link
Member

Choose a reason for hiding this comment

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

Does BehaviorSubject prevent the subscribers from running inline in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, onNext still runs inline, the stackoverflow was from the error handler. Before we were returning new completables per poll which would extend the overall subscribe from poll(url).subscribeWith(receiveLoop);. But now we only subscribe to the BehaviorSubject.onError(...) so there wont be a continuous onError subscription from new completables.

Before:
image

After:
image

Copy link
Member

Choose a reason for hiding this comment

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

I think we should dispatch this then. While it seems unlikely that polls would always complete inline, it's doesn't seem impossible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in this case it will always be dispatched because of OkHttp, but I will look into doing the dispatching ourselves

@BrennanConroy BrennanConroy merged commit b1480ca into main Jun 17, 2021
@BrennanConroy BrennanConroy deleted the brecon/refactorLP branch June 17, 2021 22:18
@ghost ghost added this to the 6.0-preview7 milestone Jun 17, 2021
jeffl8n added a commit to jeffl8n/aspnetcore that referenced this pull request Sep 18, 2021
* main:
  Handle more cases with the new entry point pattern (dotnet#33500)
  [main] Update dependencies from dotnet/runtime dotnet/efcore (dotnet#33560)
  Refactor LongPolling in Java to avoid stackoverflow (dotnet#33564)
  Optimize QueryCollection (dotnet#32829)
  Switch to in-org action (dotnet#33610)
  Improve Codespaces + C# extension interaction (dotnet#33614)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers feature-client-java Related to the SignalR Java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android SignalR Java client StackOverFlow
2 participants