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

evict okhttp connections on shutdown #3921

Merged
merged 2 commits into from
Nov 23, 2021

Conversation

svalaskevicius
Copy link
Contributor

@svalaskevicius svalaskevicius commented Nov 19, 2021

fixes OkHttp thread not exiting on shutdown:

[E] Non-daemon threads currently preventing JVM termination: - 37: Thread[DestroyJavaVM,5,main]
[E]  -  - 27: Thread[OkHttp localhost Writer,5,main]
[E]  -  - 25: Thread[OkHttp localhost,5,main]

https://square.github.io/okhttp/4.x/okhttp/okhttp3/-ok-http-client/#shutdown-isnt-necessary

Fixes #3900

@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #3921 (12f541f) into main (38d0826) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3921      +/-   ##
============================================
+ Coverage     89.87%   89.90%   +0.02%     
- Complexity     4174     4177       +3     
============================================
  Files           501      501              
  Lines         12753    12761       +8     
  Branches       1218     1218              
============================================
+ Hits          11462    11473      +11     
+ Misses          899      898       -1     
+ Partials        392      390       -2     
Impacted Files Coverage Δ
...xporter/otlp/internal/grpc/OkHttpGrpcExporter.java 84.37% <100.00%> (+0.16%) ⬆️
.../exporter/otlp/internal/okhttp/OkHttpExporter.java 85.48% <100.00%> (+0.23%) ⬆️
...exporter/otlp/logs/OtlpGrpcLogExporterBuilder.java 83.87% <0.00%> (-8.99%) ⬇️
...etry/sdk/logs/export/BatchLogProcessorBuilder.java 62.50% <0.00%> (-4.17%) ⬇️
...telemetry/sdk/trace/export/BatchSpanProcessor.java 87.94% <0.00%> (-0.71%) ⬇️
...entelemetry/sdk/logs/export/BatchLogProcessor.java 77.35% <0.00%> (ø)
...ntelemetry/sdk/extension/resources/OsResource.java 90.69% <0.00%> (+4.65%) ⬆️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 100.00% <0.00%> (+5.88%) ⬆️
...metry/sdk/extension/resources/ProcessResource.java 87.50% <0.00%> (+6.25%) ⬆️
...elemetry/sdk/extension/resources/HostResource.java 92.30% <0.00%> (+15.38%) ⬆️
... and 1 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 38d0826...12f541f. Read the comment docs.

@anuraaga
Copy link
Contributor

/easycla

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 21, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Sarunas Valaskevicius (c733fef)

@anuraaga
Copy link
Contributor

Ah also @svalaskevicius can you sign the CLA?

@svalaskevicius
Copy link
Contributor Author

Hi. Signed.

The only issue with the cla form is that I think it requires way too much info from me for a single line change :D

Eg I've used my email for the "mailing address" - IANAL, I hope it is OK (the form was accepted).

I'll try to make another pr for the other place linked later when I'm back at laptop, but feel free to add it before I do :)

@anuraaga anuraaga merged commit 576b1d2 into open-telemetry:main Nov 23, 2021
@anuraaga
Copy link
Contributor

Thanks @svalaskevicius!

anuraaga added a commit that referenced this pull request Nov 24, 2021
* evict okhttp connections on shutdown

* Evict in HTTP exporter as well

Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
This was referenced Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OkHttp Dispatcher thread keeps JVM from shutting down
3 participants