fix(python): fix SimpleBox.shutdown() crash on missing method#246
fix(python): fix SimpleBox.shutdown() crash on missing method#246DorianZheng merged 2 commits intomainfrom
Conversation
SimpleBox.shutdown() called self._box.shutdown(), but the underlying Rust Box only exposes stop(), not shutdown(). This caused: AttributeError: 'builtins.Box' object has no attribute 'shutdown' Fix: - Add async stop() method that calls self._box.stop() - Make shutdown() an async alias for stop() - Both methods are now properly async (the old shutdown was sync) Closes #229 Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in SimpleBox.shutdown() where it was calling a non-existent shutdown() method on the underlying Rust Box object, causing an AttributeError. The fix correctly calls the existing stop() method and also makes both shutdown() and the new stop() methods async, which is required since the underlying Rust methods are async.
Changes:
- Renamed the broken
shutdown()method tostop()and made it properly async - Fixed the method call from
self._box.shutdown()(doesn't exist) toawait self._box.stop()(correct) - Added
shutdown()as an async alias forstop()to maintain backward compatibility with code expecting ashutdown()method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -290,7 +290,15 @@ def shutdown(self): | |||
| "Box not started. Use 'async with SimpleBox(...) as box:' " | |||
| "or call 'await box.start()' first." | |||
| ) | |||
| self._box.shutdown() | |||
| await self._box.stop() | |||
|
|
|||
| async def shutdown(self): | |||
| """ | |||
| Shutdown the box and release resources. | |||
|
|
|||
| Alias for stop(). Usually not needed as context manager handles cleanup. | |||
| """ | |||
| await self.stop() | |||
There was a problem hiding this comment.
The synchronous wrapper SyncSimpleBox (in sdks/python/boxlite/sync_api/_simplebox.py) has a stop() method but no shutdown() method. For API consistency, consider adding a shutdown() alias in SyncSimpleBox as well, similar to what's being done here for the async SimpleBox.
| @@ -290,7 +290,15 @@ def shutdown(self): | |||
| "Box not started. Use 'async with SimpleBox(...) as box:' " | |||
| "or call 'await box.start()' first." | |||
| ) | |||
| self._box.shutdown() | |||
| await self._box.stop() | |||
|
|
|||
| async def shutdown(self): | |||
| """ | |||
| Shutdown the box and release resources. | |||
|
|
|||
| Alias for stop(). Usually not needed as context manager handles cleanup. | |||
| """ | |||
| await self.stop() | |||
There was a problem hiding this comment.
Consider adding test coverage for the new stop() and shutdown() methods. While test_simplebox.py has comprehensive tests for other SimpleBox methods like exec(), info(), and the context manager, there are no tests for explicit shutdown/stop calls. Tests should verify:
await box.stop()works without errors after starting the boxawait box.shutdown()works as an alias forstop()- Both methods raise
RuntimeErrorwhen called before starting the box - Multiple calls to
stop()/shutdown()are handled gracefully
|
Self-review: This PR looks correct. The bug: The fix:
No impact on |
Co-authored-by: Copilot <[email protected]>
Summary
SimpleBox.shutdown()callingself._box.shutdown()which doesn't exist on the RustBox— onlystop()is exposedstop()method that correctly callsself._box.stop()shutdown()an async alias forstop()Test plan
await box.shutdown()works withoutAttributeErrorawait box.stop()works as a direct alternativeCloses #229
🤖 Generated with Claude Code