Fix coroutine warning in JSONEncoder#4128
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesJSONEncoder coroutine safety
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/traceloop-sdk/traceloop/sdk/utils/json_encoder.py (1)
21-34: ⚡ Quick winConsider an
iscoroutinefunctionfast-path before invokingjson_method().The current "call then inspect" pattern correctly fixes the reported warning, but it always invokes the callable — even when we can cheaply determine ahead of time that it's async. For objects whose
jsoncallable has side effects on invocation (resource acquisition, logging, state mutation in__call__), this is unnecessary work. Checkinginspect.iscoroutinefunction(json_method)first as a fast path, and keeping the post-callinspect.iscoroutine(result)check as a fallback, gives defense-in-depth: the fast path covers the common case, and the post-call check still catches wrapped/decorated callables thatiscoroutinefunctionmisses (the case this PR explicitly motivates).♻️ Proposed defense-in-depth refactor
if callable(json_method): + if inspect.iscoroutinefunction(json_method): + return o.__class__.__name__ try: result = json_method() if inspect.iscoroutine(result): result.close() return o.__class__.__name__ return result except Exception: pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/traceloop-sdk/traceloop/sdk/utils/json_encoder.py` around lines 21 - 34, Add a cheap async fast-path before invoking the object's json callable: in json_encoder.py, when you obtain json_method (the attribute on the object), first check inspect.iscoroutinefunction(json_method) and if True, avoid calling it and return o.__class__.__name__; keep the existing try/except and the post-call inspect.iscoroutine(result) fallback intact so decorated/wrapped async callables are still detected after invocation (referencing json_method, result, and the existing exception handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/traceloop-sdk/traceloop/sdk/utils/json_encoder.py`:
- Line 39: The file packages/traceloop-sdk/traceloop/sdk/utils/json_encoder.py
is missing a trailing newline (Ruff W292); add a single newline character at EOF
so the file ends with a newline, e.g., after the final line containing "return
super().default(o)" in the default(self, o) implementation of your JSONEncoder
subclass.
---
Nitpick comments:
In `@packages/traceloop-sdk/traceloop/sdk/utils/json_encoder.py`:
- Around line 21-34: Add a cheap async fast-path before invoking the object's
json callable: in json_encoder.py, when you obtain json_method (the attribute on
the object), first check inspect.iscoroutinefunction(json_method) and if True,
avoid calling it and return o.__class__.__name__; keep the existing try/except
and the post-call inspect.iscoroutine(result) fallback intact so
decorated/wrapped async callables are still detected after invocation
(referencing json_method, result, and the existing exception handling).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c8c43ca-7b5c-4aaf-b24b-9a63281d4ba8
📒 Files selected for processing (1)
packages/traceloop-sdk/traceloop/sdk/utils/json_encoder.py
|
@cla-bot recheck |
|
Looks like this issue was already addressed in #3968 and I missed the merged PR while reading the timeline. Closing this PR |
Fixes #3967
What changed
This updates
JSONEncoderto handle objects with asyncjson()methods more safely.Previously, the encoder checked
inspect.iscoroutinefunction(json_method)before calling.json(). In the case ofstarlette.requests.Request, this check did not reliably detect the async method, which caused the coroutine to be created without being awaited and produced:RuntimeWarning: coroutine 'Request.json' was never awaitedThe updated logic checks whether the returned value is a coroutine after calling the method and closes it safely before falling back to the class name.
Verification
Tested locally using a FastAPI endpoint decorated with
@workflowand passing aRequestobject. The warning no longer appears.Summary by CodeRabbit