Patch HTTPClient to accept socket_connect_timeout #3174
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
Monkey patch HTTPClient to accept socket_connect_timeout as optional configuration. If set, it is used as connect_timeout parameter when creating a TCPSocket. Set the socket_connect_timeout in Diego::Client to (overall) connect_timeout / 2. Add test to ensure that the httpclient version is not updated without revisiting the monkey patch. Co-authored-by: Johannes Haass <johannes.haass@sap.com>
|
@johha and me prepared a local test environment to simulate an unresponsive bbs server and ran tests with different HTTP clients:
|
johha
approved these changes
Feb 3, 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.
Monkey patch
HTTPClientto acceptsocket_connect_timeoutas optional configuration. If set, it is used asconnect_timeoutparameter when creating aTCPSocket.Set the
socket_connect_timeoutinDiego::Clientto (overall)connect_timeout/ 2.Add test to ensure that the
httpclientversion is not updated without revisiting the monkey patch.Background information
ℹ️ Use case: Mitigates the risk of
Runner is unavailableandStager is unavailableerrors during a cf push when the client is unable to reach a bbs instance.❌ First, discarded solution proposal: #3002
✔️ Second, merged solution proposal: #3048
💥 Issues caused by #3048
sync-integration-testsℹ️ Root cause:
Net::HTTPand alteredDiego::Clientimplementations are not thread-safe.🔄 Revert PR: #3113
✔️ Third, merged solution proposal: #3170
💩 Issues caused by #3170
cc-unit-testsfailing (Concourse)ℹ️ Root cause: Concourse executes the unit tests with Ruby 3.1.3 (as opposed to the version 3.1.2 specified in
.ruby-version) which includes a newer Net::HTTP version that reverted the usage of theSocket.tcp'sconnect_timeoutoption (see ruby/net-http#74).🔄 Revert PR: #3171
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests