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 to hide Http MessageHandler cleanup messages #816

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

Omotola
Copy link
Contributor

@Omotola Omotola commented Sep 28, 2023

Based on these threads Stack Overflow, Github, It seems the multiple cleanup messages are default behavior as it waits to clean up the message handler resources. The suggested thing to do is hide the messages in the logs if we don't want to see them.

@Omotola Omotola requested a review from a team as a code owner September 28, 2023 05:21
@Omotola Omotola requested a review from JamieMagee September 28, 2023 05:21
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #816 (67d2fbf) into main (4560a88) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #816   +/-   ##
=======================================
  Coverage   75.84%   75.84%           
=======================================
  Files         240      240           
  Lines        9979     9979           
=======================================
  Hits         7569     7569           
  Misses       2410     2410           

@melotic
Copy link
Member

melotic commented Sep 28, 2023

This means that there's a client/message handler that's been expired, but someone is still holding a reference to it. You can analyze what's keeping it alive by capturing a memory dump.

Do we dispose all instances of the HTTPClient? Why does the old pip detector work fine but this not?

I'm not sure this is the right solution but instead just masking the logs.

@Omotola
Copy link
Contributor Author

Omotola commented Sep 29, 2023

Do we dispose all instances of the HTTPClient? Why does the old pip detector work fine but this not?

@melotic Yeah, the httpclient is only created using httpclientfactory in the SimplePypiClient, and its disposed after. The original pip detector doesn't use httpclientfactory, it manually creates the httphandler and httpclient. Jamie had mentioned that doing it that way would result in resources not being cleaned up properly afterwards. He suggested using httpclientfactory for the simplepipdetector cause it uses dependency injection to inject the httpclient, and handles the cleanup by itself.

Also if you scroll down to the end of that github issue you quoted, you'll see where they concluded that cleanup message loop is by design. I also found another article here that explains that by design httpclientfactory expires handlers after 2 minutes of no longer being in use and adds them to an expired queue, and then there is a cleanup job that checks that queue and has a timer that checks every 10 seconds if the handler has been garbage collected so it can cleanup other internal resources (i.e socket connections).
In the logs we see in our builds you will notice there is the expiration message of the one handler
image
and then the consequent cleanup loop at 10 second intervals which is just waiting for the handler to be GC
image

Copy link
Member

@melotic melotic left a comment

Choose a reason for hiding this comment

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

this will need to replicated in internal

@Omotola Omotola merged commit 56b3100 into main Sep 29, 2023
24 of 25 checks passed
@Omotola Omotola deleted the users/oakeredolu/httpcleanupmessagefix branch September 29, 2023 19:53
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.

2 participants