-
Notifications
You must be signed in to change notification settings - Fork 235
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
Improve L0_client_memory_growth test #303
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…unctions. (2) Update http_client to propogate error on client-side failure
nnshah1
reviewed
Apr 27, 2023
nnshah1
reviewed
Apr 27, 2023
nnshah1
reviewed
Apr 27, 2023
nnshah1
reviewed
Apr 27, 2023
nnshah1
reviewed
Apr 27, 2023
nnshah1
reviewed
Apr 27, 2023
…plify reuse logic, clarify HTTP millisecond timeout limitation
rmccorm4
commented
Apr 27, 2023
Tabrizian
reviewed
Apr 27, 2023
nnshah1
approved these changes
Apr 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor memory_leak_test.cc
client::Create(reuse_client. ...)
would just create new client objects under the hood.Update C++ HTTP client to correctly propogate errors on client-side curl failures. Now the behavior matches other API call failures, rather than reporting an obscure Timer and Json parsing issue. See the before/after examples below:
Before:
After:
The root cause of the connection errors above in CI was running out of ephemeral ports on the system. Sockets were being opened too quickly / too many hanging sockets were being left in WAIT state, causing an inability to assign a port to new connections.
For future reference, this can be observed by running
watch ss -s
in a separate shell while running this script. If you set the-R
flag to re-use the same client connection, you will see a consistent flat usage of sockets.If you omit this flag and create new connections on each request, you will see a consistent rise / high value in sockets stuck in
timewait
:Possible Future Improvements:
--max-retries
rather than hard-coded valueCorresponding server PR that just adds notes about these findings to help future debuggers: triton-inference-server/server#5710