fix(LibCarla): port core bug fixes from ue4-dev#9585
fix(LibCarla): port core bug fixes from ue4-dev#9585youtalk wants to merge 3 commits intocarla-simulator:ue5-devfrom
Conversation
- Fix RPC server deadlock with atomic shutdown flag (ref: 0c477d6) - Remove overly strict assert in Vector3D::MakeUnitVector (ref: 7b2ef1d) - Fix OpenDrive lane width for center lanes (ref: 3b3b890) - Replace yield() with sleep_for(100us) (ref: 4c49a74) - Relax RELEASE_ASSERT to graceful skip in road Map (ref: 5161fb0)
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes. |
There was a problem hiding this comment.
Pull request overview
Ports a set of stability and correctness fixes from ue4-dev into ue5-dev, focusing on preventing shutdown deadlocks in the RPC server, reducing overly-strict asserts, and avoiding aggressive busy-wait behavior.
Changes:
- Add a shutdown-aware wait loop for synchronous RPC calls to avoid deadlock during server shutdown.
- Replace several
std::this_thread::yield()busy-waits withsleep_for(100us)to improve scheduling behavior. - Relax/adjust validation behavior in geometry, OpenDrive lane parsing, and road waypoint generation to avoid hard crashes and missing data.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/PixelReader.cpp | Replaces yield() with sleep_for(100us) while polling GPU readback readiness. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/ImageUtil.cpp | Replaces yield() with sleep_for(100us) in async readback polling; adds required headers. |
| LibCarla/source/carla/streaming/detail/tcp/ServerSession.cpp | Replaces a synchronous-mode yield() loop with sleep_for(100us); adds <chrono>. |
| LibCarla/source/carla/rpc/Server.h | Adds shutdown flag and timeout loop in sync RPC wrapper to avoid shutdown deadlock. |
| LibCarla/source/carla/road/Map.cpp | Replaces strict RELEASE_ASSERT checks with skip/continue logic when successor/predecessor lane data is invalid. |
| LibCarla/source/carla/opendrive/parser/LaneParser.cpp | Ensures a lane width entry is created even when <width> is absent (including center lane). |
| LibCarla/source/carla/geom/Vector3D.h | Removes a strict development assert in MakeUnitVector(). |
| LibCarla/source/carla/client/detail/Simulator.cpp | Replaces yield() with sleep_for(100us) in frame-wait loop; adds <chrono>. |
| CHANGELOG.md | Documents the set of ported bug fixes in LATEST changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::future_status status; | ||
| do { | ||
| status = result.wait_for(std::chrono::milliseconds(100)); | ||
| } while (!shutdown_in_progress && (status != std::future_status::ready)); | ||
| if (status==std::future_status::ready) { | ||
| return result.get(); | ||
| } else { | ||
| return R(); | ||
| } |
There was a problem hiding this comment.
When the server is shutting down and the posted sync task never runs, this wrapper returns a default-constructed R. For RPCs returning primitive types this can silently look like a valid result (e.g., 0), and for carla::rpc::Response<T> it will surface as an unhelpful "unknown error". Consider returning a deterministic shutdown-specific error (e.g., set a ResponseError message when R is a Response<...> / Response<void>, or throw an exception that rpclib can convert into an RPC error).
| std::future_status status; | ||
| do { | ||
| status = result.wait_for(std::chrono::milliseconds(100)); | ||
| } while (!shutdown_in_progress && (status != std::future_status::ready)); | ||
| if (status==std::future_status::ready) { | ||
| return result.get(); | ||
| } else { | ||
| return R(); | ||
| } |
There was a problem hiding this comment.
This change introduces shutdown-aware waiting logic in WrapSyncCall, but there is no regression test covering the shutdown path (i.e., a client call blocked on SyncRunFor work that gets unblocked when Server::Stop() is called). Adding a unit/integration test in the existing test_rpc.cpp suite would help prevent deadlock regressions.
| @@ -66,7 +66,6 @@ namespace geom { | |||
|
|
|||
| inline Vector3D MakeUnitVector() const { | |||
| const float length = Length(); | |||
There was a problem hiding this comment.
With the DEVELOPMENT_ASSERT removed, MakeUnitVector() now always divides by length without any guard. For zero-length vectors this will produce inf/NaN components (and for very small lengths it can amplify numerical noise). Consider either restoring a weaker guard (only for length == 0 / very small) or routing callers toward MakeSafeUnitVector() for potentially-degenerate inputs.
| const float length = Length(); | |
| const float length = Length(); | |
| if (length <= 0.0f) { | |
| // Avoid division by zero for zero-length vectors; return the original vector. | |
| return *this; | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Port 5 core bug fixes from ue4-dev to ue5-dev:
Server::WrapSyncCallto prevent deadlock on shutdownDEVELOPMENT_ASSERTinMakeUnitVector()that could crash on small vectorsstd::this_thread::yield()withsleep_for(100us)for proper CPU schedulingRELEASE_ASSERTin road Map with graceful skip logicNote: Fixes 4 (GeoReferenceParser) and 5 (TrafficManager NaN) from the original plan were skipped — the ue5-dev codebase has completely different code in those areas.
Related #9583 (partial — PR #0a)
Where has this been tested?
Possible Drawbacks
None expected. All fixes are well-understood ports from the stable ue4-dev branch. The assert relaxation changes error behavior from crash to skip, which is more forgiving but could mask issues in rare edge cases.
This change is