Replace HTTParty with http.rb for persistent HTTP connections #670
Conversation
📝 WalkthroughWalkthroughReplaces HTTParty with the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPClient as "http gem client"
participant API as "Meilisearch API"
rect rgb(245,250,255)
Client->>HTTPClient: build_http_client(options, persistent?)
note right of HTTPClient: persistent:true → connection reuse
end
Client->>HTTPClient: send_request(method, path, params, body or NO_BODY)
alt body present
Client->>HTTPClient: build_request_options -> serialize body (if convert_body?)
else NO_BODY
note right of HTTPClient: omit request body
end
HTTPClient->>API: execute HTTP request
API-->>HTTPClient: response (200 / error)
alt success
HTTPClient->>Client: validate_response -> parse_response_body -> return
else transient error (HTTP::TimeoutError, connection)
HTTPClient->>Client: map error -> can_retry? -> retry with backoff
else fatal error (Errno::ECONNREFUSED, InvalidURI)
HTTPClient->>Client: raise CommunicationError / InvalidURI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/meilisearch/http_request.rb (1)
134-137: Consider treating DELETE as idempotent for retry logic.The current implementation only retries DELETE on connect timeout (when the request didn't reach the server). However, DELETE operations in REST APIs are typically idempotent—deleting the same resource multiple times has the same effect as deleting it once. Consider whether DELETE should be treated like GET for retry purposes.
# Option 1: Treat DELETE as always safe to retry (like GET) def safe_to_retry?(method, error) [:get, :delete].include?(method) || ([:post, :put, :patch].include?(method) && error.is_a?(HTTP::ConnectTimeoutError)) end # Option 2: Keep current behavior but document the reasoning # Current: Only retry DELETE on connect timeout to avoid potential issues with # non-idempotent delete implementations or cascading deletes
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdlib/meilisearch/http_request.rbmeilisearch.gemspecspec/meilisearch/client/errors_spec.rbspec/meilisearch/client/requests_spec.rbspec/meilisearch/index/base_spec.rbspec/meilisearch_spec.rb
💤 Files with no reviewable changes (1)
- spec/meilisearch/client/requests_spec.rb
🧰 Additional context used
🧬 Code graph analysis (3)
spec/meilisearch_spec.rb (1)
lib/meilisearch/client.rb (1)
indexes(68-76)
spec/meilisearch/index/base_spec.rb (3)
lib/meilisearch/client.rb (2)
index(137-139)fetch_index(146-148)lib/meilisearch/version.rb (1)
qualified_version(6-8)lib/meilisearch/index.rb (1)
fetch_info(24-28)
spec/meilisearch/client/errors_spec.rb (1)
lib/meilisearch/client.rb (1)
index(137-139)
🔇 Additional comments (18)
spec/meilisearch/client/errors_spec.rb (2)
15-15: LGTM!Typo fix improves test description clarity.
25-36: LGTM!The updated test correctly stubs the instance-level HTTP client and verifies that
Errno::EPIPEraisesCommunicationError, aligning with the new HTTP client architecture.spec/meilisearch_spec.rb (2)
39-54: LGTM!The retry test correctly validates that retryable errors (HTTP::TimeoutError) trigger the configured number of retries (3), resulting in 4 total calls. The low retry_multiplier (0.1) ensures fast test execution.
56-71: LGTM!The test correctly validates that non-retryable errors (Errno::ECONNREFUSED) do not trigger retries, regardless of the max_retries setting. This ensures the retry logic distinguishes between transient and permanent failures.
spec/meilisearch/index/base_spec.rb (2)
102-116: LGTM!The test correctly validates that options are passed through to the index and that headers are properly configured. Using
includeinstead of strict equality is more resilient to internal implementation details. Thefetch_infocall provides end-to-end validation that the configured options work correctly.
118-133: LGTM!The test correctly validates that custom client agents are combined with the default user agent in the format
"custom_agent;default_agent"and that all options work end-to-end via thefetch_infocall.lib/meilisearch/http_request.rb (10)
18-19: LGTM!The
NO_BODYsentinel constant is a clean pattern to distinguish "no body provided" from "body is nil", which is important for HTTP methods where an empty body has different semantics than no body.
10-16: LGTM!The default options are well-chosen:
persistent: falseprovides a safe default with opt-in for persistent connectionsconvert_body?: truemaintains backward compatibility with automatic JSON serialization- Retry settings (max_retries: 2, retry_multiplier: 1.2) provide reasonable defaults for transient failures
29-47: LGTM!The unified request routing through
send_requestprovides a clean abstraction that centralizes:
- Error handling
- Retry logic
- Request construction
- Response validation
The use of
NO_BODYas a default parameter for POST/PUT/PATCH methods properly distinguishes between "no body" and "nil body".
51-60: LGTM!The header construction is clean and correct:
- Conditional Authorization header based on API key presence
- Proper User-Agent combining custom agents with the library version
compactremoves nil values to avoid sending empty headers
67-73: LGTM!The URL construction logic correctly handles both modes:
- Persistent mode: uses relative paths (client already bound to base_url)
- Non-persistent mode: prepends base_url to relative path
Options merging allows per-request overrides while preserving instance defaults.
94-96: LGTM!Clear retry gate that combines attempt count with method/error safety checks.
113-126: LGTM!Response validation and parsing are well-structured:
- Non-success responses raise
ApiErrorwith status and body- Empty responses return nil (avoiding parse errors)
- JSON parse failures gracefully fall back to raw body string
- This maintains backward compatibility with existing code expecting parsed JSON responses
128-132: LGTM!The URI error handling provides a helpful error message when the protocol is missing, which is a common mistake (e.g., using
"localhost:7700"instead of"http://localhost:7700").
98-111: No action needed. The http.rb gem handles nil body values correctly—when a request body is nil or omitted, it simply sends no payload rather than erroring. The code correctly setsrequest_opts[:body] = niland http.rb will handle it as expected.Likely an incorrect or invalid review comment.
75-92: Error handling is complete and appropriate as-is.The error handling correctly covers all exceptions that http.rb raises in its default behavior. Since the code does not enable http.rb's RaiseError feature, HTTP::ResponseError and HTTP::StatusError are never raised—response errors are instead handled manually via validate_response() which checks status codes and raises custom ApiError objects. The current catch blocks appropriately handle connection errors, timeouts, and URI validation errors that can occur in normal operation.
README.md (1)
99-108: No changes needed. The documentation is accurate. HTTP.rb version 5.0 supports persistent connections, and the Meilisearch-ruby client correctly implements this feature by calling thepersistent()method on the HTTP client instance, as shown inlib/meilisearch/http_request.rb(client.persistent(@base_url)). The{ persistent: true }option is properly handled and defaults tofalse.meilisearch.gemspec (1)
18-18: The http gem version 5.0 exists, is stable, and has no known security vulnerabilities.Version 5.0.0 was released in May 2021 and is marked stable. Multiple patch releases (5.0.1–5.0.4) followed, confirming stability. No CVEs or security advisories have been published for version 5.0.x. The dependency specification '~> 5.0' is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/meilisearch/http_request.rb (1)
108-112: Clarify the nil body behavior.When
bodyis explicitlynil(notNO_BODY),nil.to_jsonproduces"null"which will be sent as the request body. This is technically correct given the sentinel pattern, but could be surprising.Consider adding a brief comment explaining this intentional distinction, or handle
nilspecially if sending"null"as a body is never a valid use case for Meilisearch.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/meilisearch/http_request.rb
🧰 Additional context used
🧬 Code graph analysis (1)
lib/meilisearch/http_request.rb (2)
lib/meilisearch/error.rb (8)
attr_reader(7-53)attr_reader(55-62)attr_reader(64-71)attr_reader(73-80)initialize(23-33)initialize(58-61)initialize(67-70)initialize(76-79)lib/meilisearch/utils.rb (1)
parse(84-90)
🔇 Additional comments (7)
lib/meilisearch/http_request.rb (7)
7-12: Good documentation for thread safety considerations.The thread safety documentation addresses the concern from the previous review. Clear guidance for users to create separate client instances per thread when using persistent connections.
24-25: Good use of sentinel pattern for body handling.Using
NO_BODY = Object.new.freezeas a sentinel is a clean way to distinguish between "no body passed" and "body is explicitly nil". This enables proper handling of edge cases in HTTP methods.
35-53: Clean unified request flow.The refactoring to route all HTTP methods through
send_requestwith a consistent interface is a good design improvement over the previous implementation.
81-98: Solid error handling and retry logic.The error handling correctly maps HTTP library exceptions to Meilisearch-specific errors. The retry logic with exponential backoff (
retry_multiplier ** attempts) is well-implemented, and thecan_retry?check ensures safe retry semantics.
119-132: Good response handling with graceful fallback.The response validation correctly uses
ApiErrorwith the expected parameters. The JSON parsing with fallback to raw body handles edge cases like non-JSON responses gracefully.
140-143: Correct retry safety semantics.The
safe_to_retry?method correctly implements retry safety: GET requests are idempotent and always safe to retry, while POST/PUT/PATCH/DELETE are only safe to retry onConnectTimeoutError(when the request never reached the server). This prevents duplicate side effects from non-idempotent operations.
73-79: Line number correction: The code referenced is at lines 105-110, not 73-79.The URL concatenation concern does not apply in practice. Throughout the codebase, relative paths consistently start with
/(e.g.,/indexes,/swap-indexes) while the base URL never ends with/(e.g.,http://127.0.0.1:8800). This convention ensures correct concatenation:http://127.0.0.1:8800 + /indexes = http://127.0.0.1:8800/indexes.Likely an incorrect or invalid review comment.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/meilisearch/http_request.rb (1)
157-164: Minor: Redundant nil check.Since
response.body.to_swill never returnnil(it returns an empty string for empty bodies), thebody.nil?check on line 159 is unnecessary. However, it doesn't affect correctness.🔎 Optional simplification
def parse_response_body(response) body = response.body.to_s - return nil if body.nil? || body.empty? + return nil if body.empty? JSON.parse(body) rescue JSON::ParserError body end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/meilisearch/http_request.rb
🧰 Additional context used
🧬 Code graph analysis (1)
lib/meilisearch/http_request.rb (1)
lib/meilisearch/error.rb (8)
attr_reader(7-53)attr_reader(55-62)attr_reader(64-71)attr_reader(73-80)initialize(23-33)initialize(58-61)initialize(67-70)initialize(76-79)
🔇 Additional comments (9)
lib/meilisearch/http_request.rb (9)
16-26: LGTM - Good use of sentinel pattern and conservative defaults.The
NO_BODYsentinel is a clean pattern for distinguishing "no body passed" from "body is explicitly nil". Thepersistent: falsedefault is appropriately conservative for opt-in behavior.
44-85: LGTM - Clean unified API design.The public HTTP methods follow a consistent pattern, properly delegate to
send_request, and the YARD documentation clearly describes the parameters and return types.
89-98: LGTM - Headers built correctly.The conditional Authorization header and User-Agent construction are well-implemented. The
.compactcall cleanly removes nil values.
100-103: Thread safety documentation addresses previous concern.The implementation correctly builds the HTTP client with optional persistent connections. The thread safety note at the class level (lines 9-12) properly documents the expected usage pattern in multi-threaded environments.
105-111: LGTM - URL construction handles persistent vs non-persistent correctly.The URL is correctly built as just the relative path for persistent connections (since
http.rb's persistent client already has the base URL), and as the full URL otherwise.
113-130: LGTM - Error handling and retry logic are well-structured.The error handling correctly maps various exceptions to appropriate Meilisearch error types. The retry logic properly tracks attempts and uses exponential backoff via
retry_multiplier.
132-134: Well-designed retry safety check.The
safe_to_retry?method correctly handles idempotency concerns: GET requests can always be retried, while non-GET methods are only retried onConnectTimeoutError(when the request hasn't reached the server). This prevents accidental duplicate mutations from retry logic.Also applies to: 172-175
136-149: LGTM - Request options built correctly.The
NO_BODYsentinel check using.equal?ensures only the exact sentinel object triggers the "no body" path, while explicitnilbody values are properly JSON-serialized. The conditional header override is clean.
166-170: LGTM - Helpful URI error messaging.The scheme detection and suggested fix ("Did you mean https://...") provides a much better developer experience for the common mistake of omitting the URL scheme.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/meilisearch/http_request.rb (1)
157-164: Minor:body.nil?check is redundant.At line 159,
body.nil?will never be true becauseresponse.body.to_sat line 158 always returns a string (empty string at minimum, nevernil). Thebody.empty?check alone is sufficient.🔎 Suggested simplification
def parse_response_body(response) body = response.body.to_s - return nil if body.nil? || body.empty? + return nil if body.empty? JSON.parse(body) rescue JSON::ParserError body endREADME.md (1)
99-108: Good documentation for the new feature.The persistent connections section is well-placed and clearly explains:
- When to use it (high-throughput scenarios)
- How to enable it
- What benefit it provides
Consider adding a note about thread safety (creating separate client instances per thread) as mentioned in the code comments, especially since this is relevant for the high-throughput use cases where persistent connections shine.
🔎 Optional: Add thread safety note
This uses `http.rb`'s built-in persistent connection support, reducing the overhead of establishing new connections for each request. + +> **Note:** In multi-threaded environments (Puma, Sidekiq), create a separate `Client` instance per thread rather than sharing one across threads.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdlib/meilisearch/http_request.rbmeilisearch.gemspecspec/meilisearch/client/errors_spec.rbspec/meilisearch/client/requests_spec.rbspec/meilisearch/index/base_spec.rbspec/meilisearch_spec.rb
💤 Files with no reviewable changes (1)
- spec/meilisearch/client/requests_spec.rb
🧰 Additional context used
🧬 Code graph analysis (3)
spec/meilisearch/client/errors_spec.rb (1)
lib/meilisearch/client.rb (1)
index(137-139)
spec/meilisearch/index/base_spec.rb (3)
lib/meilisearch/client.rb (2)
index(137-139)create_index(95-101)lib/meilisearch/version.rb (1)
qualified_version(6-8)lib/meilisearch/index.rb (1)
fetch_info(24-28)
spec/meilisearch_spec.rb (1)
lib/meilisearch/client.rb (1)
indexes(68-76)
🔇 Additional comments (15)
lib/meilisearch/http_request.rb (9)
24-26: Good use of sentinel pattern for NO_BODY.Using
Object.new.freezeas a sentinel value effectively distinguishes between "no body provided" and "body is explicitly nil". This is a clean pattern that prevents ambiguity in the API.
44-85: Clean centralized request flow.The HTTP method implementations properly delegate to
send_requestwith consistent signatures. The use ofNO_BODYas default for body-accepting methods (POST/PUT/PATCH) is appropriate.
100-103: Persistent connection implementation is correct.The conditional wrapping with
.persistent(@base_url)properly enables connection reuse when the option is set. Thread safety concerns were addressed with the documentation note at lines 9-12.
105-111: URL handling for persistent connections is correct.When using persistent connections, http.rb expects relative paths since the base URL is already set on the client. The conditional at line 107 correctly handles both cases.
132-134: Retry count logic is correct.With
max_retries: 3, the conditionattempts <= options[:max_retries]allows retries on attempts 1, 2, and 3, resulting in 4 total calls (1 initial + 3 retries), which matches the test expectations.
166-170: Helpful error message for missing URL scheme.The check at line 167 provides a user-friendly suggestion when the URL is missing a protocol, guiding users to add
https://. This improves the developer experience.
172-175: Correct retry safety logic for non-idempotent requests.The implementation correctly identifies that:
- GET requests are always safe to retry (idempotent)
- POST/PUT/PATCH/DELETE only retry on
ConnectTimeoutErrorwhen the connection was never established, preventing duplicate writesThis follows best practices for retry logic with non-idempotent operations.
140-144: Verify if explicitly passingnilbody (serialized to"null") is correct for the Meilisearch API.The code intentionally distinguishes between
NO_BODY(no body sent) and an explicitly passednil(body becomes the string"null"). This is confirmed by the sentinel design and comment at lines 141-142. However, there is at least one call site (lib/meilisearch/task.rb:32) that explicitly passesniltohttp_post, which would result in sending"null"as the request body. Verify with the Meilisearch API documentation or integration tests whether this behavior is correct, or if explicitly passingnilshould skip sending a body entirely.
119-129: The retry logic correctly handlesHTTP::ConnectTimeoutError. The HTTP gem's exception hierarchy confirms thatHTTP::ConnectTimeoutErroris a direct subclass ofHTTP::TimeoutError, so the rescue block at line 121 properly catches both connection timeouts and other timeout errors. Thesafe_to_retry?method then selectively retries based on the request method—allowing retries for all timeouts on GET requests, but only forHTTP::ConnectTimeoutErroron non-idempotent methods (POST, PUT, PATCH, DELETE). This is correct and intentional.spec/meilisearch/client/errors_spec.rb (1)
25-36: Test correctly validates EPIPE error handling.The test properly:
- Accesses the internal
@http_clientinstance variable- Stubs the
postmethod to raiseErrno::EPIPE- Verifies that
CommunicationErroris raised to the callerThis aligns with the centralized error handling in
execute_request.spec/meilisearch/index/base_spec.rb (2)
102-116: Improved test approach with actual HTTP calls.The refactored test now:
- Verifies options are correctly merged (including
convert_body?: true)- Checks headers are properly constructed
- Actually exercises
fetch_infoto validate end-to-end functionalityThis is a more robust integration test compared to mocking individual HTTP calls.
118-133: Custom agent test validates User-Agent composition.The test correctly verifies that custom agents are prepended to the User-Agent header with the expected format:
custom_agent;Meilisearch.qualified_version.spec/meilisearch_spec.rb (2)
39-54: Retry test properly validates retry behavior.The test correctly:
- Configures
max_retries: 3andretry_multiplier: 0.1(fast backoff for tests)- Stubs to raise
HTTP::TimeoutErroron every call- Verifies exactly 4 calls occur (1 initial + 3 retries)
- Confirms
Meilisearch::TimeoutErroris ultimately raised
56-71: Non-retryable error test validates no retry on connection refused.The test correctly demonstrates that
Errno::ECONNREFUSEDis not retried (only 1 call) despitemax_retries: 10, confirming that connection errors bypass the retry logic and are immediately wrapped asCommunicationError.meilisearch.gemspec (1)
18-18: Dependency change looks appropriate.The switch from
httpartytohttp(~> 5.0) aligns with the PR objectives. The pessimistic version constraint appropriately allows patch/minor updates within 5.x while preventing breaking changes from 6.0+. The http gem is actively maintained, with the latest version at 5.3.1.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
===========================================
- Coverage 100.00% 99.87% -0.13%
===========================================
Files 10 10
Lines 809 821 +12
===========================================
+ Hits 809 820 +11
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi there, I really appreciate the effort here, thanks a lot :) Before moving forward or not with your PR I wanted to know if your workload suffer from consistent-random timeouts along the way? I considered (even with a temp branch) this gem to replace httparty in an attempt to fix the issue meilisearch/meilisearch-rails#355 but no luck. |
2 participants
Summary
This PR replaces HTTParty with http.rb as the HTTP client library. The http.rb gem provides built-in support for persistent HTTP connections, which allows TCP connection reuse across multiple requests in high-throughput scenarios.
Changes
httpartydependency withhttp(~> 5.0) in gemspecHTTPRequestclass to use http.rb APIpersistent: trueclient option to enable connection reuseUsage
Why http.rb?
Checklist
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.