fix: Improve JSON parsing robustness under high concurrency - #1067
fix: Improve JSON parsing robustness under high concurrency#1067Asthenia0412 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors tokenizer management to support multiple models, improves request body parsing error handling, and integrates semantic cache checking. However, several critical issues were identified in the review: calling 'tokenize_prompt' in the parent router class will raise an AttributeError; raising a ValueError for missing models will break multipart/form-data requests; catching the wrong JSONDecodeError and using the incorrect 'status' parameter in HTTPException will cause runtime errors; unhandled JSON parsing errors in the semantic cache check could lead to 500 errors; and calling the blocking 'AutoTokenizer.from_pretrained' on the event loop will degrade performance.
| remote_url, headers=headers, json=data, timeout=10 | ||
| ).json() | ||
| token_ids = body["tokens"] | ||
| token_ids = await self.tokenize_prompt(endpoints, request_json) |
There was a problem hiding this comment.
Calling self.tokenize_prompt here will raise an AttributeError when using KvawareRouter directly. KvawareRouter is the parent class of LoadAwareRouter, but tokenize_prompt is defined only in the subclass LoadAwareRouter (line 630). To fix this, tokenize_prompt should be moved to the parent class KvawareRouter so that both routers can access it.
| model_name = request_json.get("model", "") | ||
| if not model_name: | ||
| raise ValueError("Missing 'model' in request body") |
There was a problem hiding this comment.
Raising ValueError when model is missing from request_json will break multipart/form-data requests (such as audio transcriptions and image edits), which call route_request with an empty dict {} as request_json (see line 1308 in request.py). Instead of raising an error, fall back to endpoints[0].model_names[0] if endpoints is provided, to maintain compatibility with the original behavior.
model_name = request_json.get("model", "")
if not model_name:
if endpoints:
model_name = endpoints[0].model_names[0]
else:
raise ValueError("Missing 'model' in request body and no endpoints available")| is_streaming = request_json.get("stream", False) | ||
| model_name = request_json.get("model", "unknown") | ||
| except (JSONDecodeError, UnicodeDecodeError, ValueError): | ||
| except (JSONDecodeError, UnicodeDecodeError, ValueError) as e: |
There was a problem hiding this comment.
The JSONDecodeError caught here is imported from requests (line 27), but json.loads(body) raises json.JSONDecodeError. Since json.JSONDecodeError is not a subclass of requests.JSONDecodeError, standard JSON decoding errors will not be caught here and will result in a 500 Internal Server Error. Change JSONDecodeError to json.JSONDecodeError to correctly catch parsing failures.
| except (JSONDecodeError, UnicodeDecodeError, ValueError) as e: | |
| except (json.JSONDecodeError, UnicodeDecodeError, ValueError) as e: |
| raise HTTPException( | ||
| status=400, | ||
| detail=f"Request body is not JSON parsable: {e}", | ||
| ) |
There was a problem hiding this comment.
fastapi.HTTPException does not accept a status parameter; it expects status_code. Using status=400 will raise a TypeError at runtime. Change it to status_code=400.
| raise HTTPException( | |
| status=400, | |
| detail=f"Request body is not JSON parsable: {e}", | |
| ) | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Request body is not JSON parsable: {e}", | |
| ) |
| # Get the request body | ||
| body = await request.json() | ||
| body = await request.body() | ||
| body = json.loads(body) if body else {} |
There was a problem hiding this comment.
If the request body is malformed or truncated, json.loads(body) will raise a json.JSONDecodeError. Since this is not caught within check_semantic_cache, it will bubble up and cause a 500 Internal Server Error. Wrapping it in a try-except block and returning None allows the request to proceed to the main handler, which will gracefully return a 400 Bad Request with detailed diagnostics.
| body = json.loads(body) if body else {} | |
| try: | |
| body = json.loads(body) if body else {} | |
| except (json.JSONDecodeError, UnicodeDecodeError): | |
| logger.warning("Failed to parse request body as JSON in check_semantic_cache") | |
| return None |
| if model_name not in self.tokenizers: | ||
| self.tokenizers[model_name] = AutoTokenizer.from_pretrained( | ||
| model_name | ||
| ) |
There was a problem hiding this comment.
AutoTokenizer.from_pretrained is a synchronous, blocking operation that performs disk and potentially network I/O. Calling it directly on the event loop will block all concurrent requests. To improve robustness under high concurrency, run this blocking call in an executor.
if model_name not in self.tokenizers:
loop = asyncio.get_running_loop()
self.tokenizers[model_name] = await loop.run_in_executor(
None, lambda: AutoTokenizer.from_pretrained(model_name)
)b4551a9 to
247a453
Compare
Under high concurrency, the router could encounter JSONDecodeError
("Unterminated string") when parsing request bodies. The root cause is
a race window where the request body can be read in a truncated state,
particularly when `request.json()` is called before `request.body()`
in the semantic cache integration path.
Changes:
1. Add diagnostic logging in `route_general_request` when JSON parsing
fails — logs actual body length, Content-Length header, and the
parser error to help debug truncation issues
2. Return detailed error info (error detail + body_length) in the 400
response so callers can diagnose the problem
3. Add same diagnostic logging in `process_request` for consistency
4. Replace `request.json()` with `request.body()` + `json.loads()` in
`check_semantic_cache` to avoid double body reading and ensure
consistent body access pattern
Closes vllm-project#369
Signed-off-by: Asthenia <asthenia0412@gmail.com>
Signed-off-by: Yancy <asthenia0412@gmail.com>
- Fix HTTPException: use status_code=400 instead of status=400 - Use json.JSONDecodeError instead of requests.JSONDecodeError for correct exception type matching - Wrap json.loads in check_semantic_cache with try-except, return None on parse failure to gracefully fall through to main handler Signed-off-by: Asthenia <asthenia0412@gmail.com> Signed-off-by: Yancy <asthenia0412@gmail.com>
247a453 to
95c0768
Compare
Description
Fixes #369
Under high concurrency, the router could encounter
JSONDecodeError("Unterminated string") when parsing request bodies. The root cause is a race window where the request body can be read in a truncated state, particularly whenrequest.json()is called beforerequest.body()in the semantic cache integration path.Changes
route_general_request: When JSON parsing fails, log the actual body length,Content-Lengthheader, and the parser error to help debug truncation issuesdetail(error message) andbody_lengthin the 400 response so callers can diagnose the problemprocess_request: Added same diagnostic logging for consistencycheck_semantic_cachefix: Replacedrequest.json()withrequest.body()+json.loads()to avoid double body reading and ensure consistent body access patternTesting
Content-Lengthheader is compared against actual body length to detect truncationCloses #369