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 potential unobserved exception in InputHandler.cs #635

Conversation

ycholette
Copy link
Contributor

Observable.Do() does not catch exceptions from FromAsync(), resulting in a potential unobserved exception.

@dnfadmin
Copy link

dnfadmin commented Aug 12, 2021

CLA assistant check
All CLA requirements met.

@github-actions github-actions bot added this to the v0.19.3 milestone Aug 12, 2021
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #635 (999977f) into master (5281160) will decrease coverage by 3.54%.
The diff coverage is 100.00%.

❗ Current head 999977f differs from pull request most recent head e70890d. Consider uploading reports for the commit e70890d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #635      +/-   ##
==========================================
- Coverage   72.89%   69.34%   -3.55%     
==========================================
  Files         257      257              
  Lines       12576    12583       +7     
  Branches      848      848              
==========================================
- Hits         9167     8726     -441     
- Misses       3409     3587     +178     
- Partials        0      270     +270     
Impacted Files Coverage Δ
src/JsonRpc/InputHandler.cs 85.33% <100.00%> (-3.98%) ⬇️
src/JsonRpc/ExternalServiceProvider.cs 50.00% <0.00%> (-50.00%) ⬇️
src/JsonRpc.Generators/SymbolExtensions.cs 71.42% <0.00%> (-28.58%) ⬇️
src/JsonRpc/Server/ContentModifiedException.cs 37.50% <0.00%> (-18.75%) ⬇️
src/JsonRpc/Server/RequestCancelledException.cs 37.50% <0.00%> (-18.75%) ⬇️
...rver/Configuration/ChainedConfigurationProvider.cs 58.62% <0.00%> (-17.25%) ⬇️
...ient/Configuration/ChainedConfigurationProvider.cs 0.00% <0.00%> (-17.25%) ⬇️
...rver/Configuration/ChainedConfigurationProvider.cs 0.00% <0.00%> (-17.25%) ⬇️
src/JsonRpc/Server/ServerErrorResult.cs 83.33% <0.00%> (-16.67%) ⬇️
src/JsonRpc.Generators/Contexts/SyntaxSymbol.cs 75.00% <0.00%> (-16.67%) ⬇️
... and 97 more

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 5281160...e70890d. Read the comment docs.

@david-driscoll
Copy link
Member

Is there an example case where you're seeing this as an issue?

Is there any docs or source you can point to? I would have assumed any exception would just go to the error flow.

@ycholette
Copy link
Contributor Author

I think this is related to this: dotnet/reactive#1256
Basically, the cancelation token was cancelled, _pipeReader.AdvanceTo(buffer.Start, buffer.End) threw an exception (before the check on the token), and the exception was not observed by anyone. My break point in the Do() never hit.

@david-driscoll
Copy link
Member

Alright, that makes a little more sense! Thanks!

@david-driscoll david-driscoll merged commit de1591e into OmniSharp:master Aug 24, 2021
@github-actions github-actions bot added the mysterious We forgot to label this label Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysterious We forgot to label this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants