refactor and pybind of OnlineWebsocketServer#1943
refactor and pybind of OnlineWebsocketServer#1943manickavela29 wants to merge 1 commit intok2-fsa:masterfrom
Conversation
|
Hi @csukuangfj, |
|
Have you compiled and tested it locally? Does it build successfully and also run successfully for you? |
|
yes, it is functional in my local testing with CUDA provider, when I raised PR, observed that for some android build, it was failing with '#include "asio"', |
In that case, I suggest that you move the note that |
|
I have moved the WebSocket file to python/src object file and cleaned it, let me know if it is good enough |
|
Thanks for the suggestions, I will address them this week. |
- Create OnlineWebsocketServerApp class with proper separation of library and executable code - Add online-websocket-server.h header file for public interface - Add online-websocket-server-app.cc with class implementation and StartServer() function for Python bindings - Update online-websocket-server.cc to only contain main() - Add websocket sources to sherpa-onnx-core library conditionally - Create Python pybind wrapper (online-websocket-server-app.cc/h)
manickavela-uni
force-pushed
the
py_ws_server
branch
from
109c6ce to
e7d617d
Compare
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughThis PR introduces a refactored WebSocket server application infrastructure for sherpa-onnx. It establishes OnlineWebsocketServerApp as a dedicated lifecycle-management class, moves WebSocket compilation flags from global to target-specific definitions, and exposes the server functionality through Python bindings via start_online_websocket_server. The changes reorganize server initialization, signal handling, and thread management into a cohesive application class. Changes
Sequence Diagram(s)sequenceDiagram
participant Python
participant PyBinding as Python<br/>Bindings
participant App as OnlineWebsocket<br/>ServerApp
participant ASIO as ASIO<br/>I/O Context
participant Threads as Thread<br/>Pools
participant Signal as Signal<br/>Handler
Python->>PyBinding: start_online_websocket_server(args)
activate PyBinding
PyBinding->>PyBinding: Convert list to argc/argv
PyBinding->>App: StartServer(argc, argv)
deactivate PyBinding
activate App
App->>App: Parse command-line options
App->>App: Initialize server config
App->>Signal: Register SIGINT/SIGTERM handlers
activate Signal
App->>ASIO: Create io_context instances
rect rgba(100, 150, 200, 0.2)
Note over App,Threads: Thread Pool Setup
App->>Threads: Create io_threads_ pool
App->>Threads: Create work_threads_ pool
end
App->>ASIO: Start server on configured port
App->>Threads: Run asio.run() in thread pools
Note over App: Blocks until Stop() called
alt User sends SIGINT/SIGTERM
Signal->>App: Call Stop()
App->>App: Set shutdown flag
App->>ASIO: Stop io_context
App->>Threads: Join all threads
end
deactivate Signal
deactivate App
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
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 |
|
@gemini-code-assist Please review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @sherpa-onnx/csrc/online-websocket-server-app.cc:
- Line 40: The global pointer g_server_app is assigned in
OnlineWebsocketServerApp's constructor but only cleared at the end of Run(),
causing a dangling pointer if Run() returns early; fix by ensuring g_server_app
is cleared on all exit paths (e.g., set g_server_app = nullptr before each early
return in Run() at the exits currently at lines like 81 and 89) and/or implement
RAII by adding OnlineWebsocketServerApp::~OnlineWebsocketServerApp() that clears
g_server_app when this == g_server_app so the global is always nulled when the
object is destroyed.
- Line 40: The global g_server_app pointer is accessed without synchronization
(assigned in StartServer and read in SignalHandler) and lacks documentation
about the single-instance requirement; update the code to either make
g_server_app a std::atomic<OnlineWebsocketServerApp*> or add a static guard to
enforce single-instance creation, and add a comment explaining why only one
instance is allowed; specifically, change the global declaration of g_server_app
to an atomic pointer (or add a creation guard in StartServer) and ensure
SignalHandler reads it atomically, and add a brief comment above g_server_app
referencing StartServer, SignalHandler, and Stop (which already uses
.exchange()) to explain the concurrency model.
- Around line 140-143: StartServer currently constructs a local
OnlineWebsocketServerApp and calls Run(), which can leave g_server_app dangling
if Run() throws; either (A) make OnlineWebsocketServerApp's destructor clear
g_server_app so RAII ensures safety on exception, or (B) wrap the
construction/Run() call in a try-catch that clears g_server_app in the catch
(and rethrows or handle), referencing StartServer, OnlineWebsocketServerApp,
Run, and g_server_app so the global pointer is always reset even on exceptions.
- Around line 42-47: SignalHandler currently calls non-async-signal-safe
functions (SHERPA_ONNX_LOGE and g_server_app->Stop()), which is unsafe; replace
that by using an async-safe mechanism: either (preferred) remove usage of
SignalHandler and register an asio::signal_set in Run() (e.g., asio::signal_set
signals(io_conn_, SIGINT, SIGTERM); signals.async_wait([this](auto, auto){
Stop(); });) or make SignalHandler only set an atomic flag (e.g.,
std::atomic<bool> g_shutdown_requested) or write a byte to a self-pipe, and
change Run()/the IO loop to poll that flag or pipe and call Stop() from the main
thread; ensure you stop calling SHERPA_ONNX_LOGE or g_server_app->Stop() from
inside SignalHandler itself.
🧹 Nitpick comments (3)
sherpa-onnx/csrc/online-websocket-server.h (1)
17-34: Missingsherpa_onnxnamespace for class and function declarations.The
OnlineWebsocketServerAppclass andStartServerfunction are declared in the global namespace, but the implementation inonline-websocket-server-app.ccuses types fromsherpa_onnxnamespace (e.g.,sherpa_onnx::ParseOptions,sherpa_onnx::OnlineWebsocketServerConfig). For consistency with the codebase conventions, consider wrapping these declarations in thesherpa_onnxnamespace.🔎 Proposed fix
#include "sherpa-onnx/csrc/parse-options.h" +namespace sherpa_onnx { + class OnlineWebsocketServerApp { public: OnlineWebsocketServerApp(int32_t argc, char *argv[]); void Run(); void Stop(); private: int32_t argc_; char **argv_; asio::io_context io_conn_; // ASIO context for connections asio::io_context io_work_; // ASIO context for work std::atomic<bool> shutdown_requested_{false}; std::vector<std::thread> io_threads_; std::vector<std::thread> work_threads_; }; // Declare StartServer so it's accessible for Pybind void StartServer(int32_t argc, char *argv[]); +} // namespace sherpa_onnx + #endif // SHERPA_ONNX_ONLINE_WEBSOCKET_SERVER_HNote: If you apply this change, update
online-websocket-server.ccto usesherpa_onnx::OnlineWebsocketServerApp.sherpa-onnx/csrc/online-websocket-server-app.cc (1)
106-106: Clarify: Work guard created as local variable.The
work_guard(line 106) is created as a local variable and will be destroyed at the end ofRun(). This is likely intentional—the guard keepsio_work_alive while work threads are running, and its destruction allows the context to finish naturally afterio_conn_.stop()is called. However, the brief lifetime might be worth a comment for future maintainers.Suggested comment
// give some work to do for the io_work pool + // Note: work_guard is destroyed at scope exit, allowing io_work_ to complete naturally auto work_guard = asio::make_work_guard(io_work_);sherpa-onnx/csrc/CMakeLists.txt (1)
452-459: Suggest optional refactor: Document the WebSocket feature flag propagation.The
SHERPA_ONNX_ENABLE_WEBSOCKET=1definition is correctly markedPUBLIC, allowing dependent code (including Python bindings) to conditionally compile WebSocket features. Consider adding a comment explaining this propagation, as it's critical for the Python bindings to work correctly.if(SHERPA_ONNX_ENABLE_WEBSOCKET) target_compile_definitions(sherpa-onnx-core PRIVATE ASIO_STANDALONE) target_compile_definitions(sherpa-onnx-core PRIVATE _WEBSOCKETPP_CPP11_STL_) + # PUBLIC flag enables conditional compilation in Python bindings and other consumers target_compile_definitions(sherpa-onnx-core PUBLIC SHERPA_ONNX_ENABLE_WEBSOCKET=1) if(NOT WIN32) target_compile_options(sherpa-onnx-core PRIVATE -Wno-deprecated-declarations) endif() endif()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
sherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/online-websocket-server-app.ccsherpa-onnx/csrc/online-websocket-server.ccsherpa-onnx/csrc/online-websocket-server.hsherpa-onnx/python/csrc/CMakeLists.txtsherpa-onnx/python/csrc/online-websocket-server-app.ccsherpa-onnx/python/csrc/online-websocket-server-app.hsherpa-onnx/python/csrc/sherpa-onnx.ccsherpa-onnx/python/sherpa_onnx/__init__.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/python/csrc/CMakeLists.txtsherpa-onnx/python/csrc/sherpa-onnx.ccsherpa-onnx/csrc/CMakeLists.txt
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
sherpa-onnx/python/csrc/CMakeLists.txtsherpa-onnx/python/csrc/sherpa-onnx.ccsherpa-onnx/csrc/CMakeLists.txt
🧬 Code graph analysis (5)
sherpa-onnx/python/csrc/online-websocket-server-app.h (1)
sherpa-onnx/python/csrc/online-websocket-server-app.cc (2)
PybindOnlineWebsocketServerApp(33-37)PybindOnlineWebsocketServerApp(33-33)
sherpa-onnx/csrc/online-websocket-server-app.cc (1)
sherpa-onnx/csrc/online-websocket-server.cc (1)
app(9-9)
sherpa-onnx/python/csrc/online-websocket-server-app.cc (1)
sherpa-onnx/csrc/online-websocket-server-app.cc (2)
StartServer(140-143)StartServer(140-140)
sherpa-onnx/python/csrc/sherpa-onnx.cc (1)
sherpa-onnx/python/csrc/online-websocket-server-app.cc (2)
PybindOnlineWebsocketServerApp(33-37)PybindOnlineWebsocketServerApp(33-33)
sherpa-onnx/csrc/online-websocket-server.cc (1)
sherpa-onnx/csrc/online-websocket-server-app.cc (1)
app(141-141)
🪛 Flake8 (7.3.0)
sherpa-onnx/python/sherpa_onnx/__init__.py
[error] 88-88: 'sherpa_onnx.lib._sherpa_onnx.start_online_websocket_server' imported but unused
(F401)
🔇 Additional comments (11)
sherpa-onnx/python/sherpa_onnx/__init__.py (1)
85-90: LGTM - Optional websocket import pattern is correct.The try/except pattern correctly handles builds without websocket support. The Flake8 F401 warning is a false positive since this is an intentional re-export for the module's public API.
Consider adding an explicit
__all__list or usingas start_online_websocket_serverto make the re-export intent clearer and silence the linter, but this is optional.sherpa-onnx/python/csrc/online-websocket-server-app.h (1)
1-16: LGTM!The header guard now correctly matches the filename, and the declaration is clean and minimal.
sherpa-onnx/python/csrc/CMakeLists.txt (1)
92-105: LGTM!The conditional websocket source inclusion and compile definitions follow the established pattern used for TTS and speaker diarization features. Using
PRIVATEfor compile definitions is appropriate since these don't need to propagate to dependents.sherpa-onnx/python/csrc/sherpa-onnx.cc (1)
48-50: LGTM!The conditional include and binding registration follow the established pattern for optional features. Unlike TTS and speaker diarization which define
py::none()placeholders when disabled, this feature relies on the Python-side try/except in__init__.pyto handle the missing symbol—which is a valid alternative approach.Also applies to: 123-126
sherpa-onnx/csrc/online-websocket-server.cc (1)
8-11: LGTM - Clean refactor of main().The main function is now cleanly delegating all responsibilities to
OnlineWebsocketServerApp, which handles option parsing, signal registration, server lifecycle, and thread management. This separation enables reuse via the Python bindings throughStartServer.sherpa-onnx/python/csrc/online-websocket-server-app.cc (1)
14-31: The code is correct. The project uses C++17 (as configured in CMakeLists.txt withset(CMAKE_CXX_STANDARD 17)), where non-conststd::string::data()returnschar*, making the implicit conversion on line 26 valid.Likely an incorrect or invalid review comment.
sherpa-onnx/csrc/online-websocket-server-app.cc (2)
130-138: LGTM: Idempotent shutdown with atomic exchange.The
Stop()method correctly usesshutdown_requested_.exchange(true)to ensure idempotent shutdown, preventing duplicate stop calls from racing. The method safely stops both I/O contexts.
98-98: All member variables are properly declared in the header file with correct types:
io_conn_andio_work_areasio::io_contextinstances (lines 26-27)shutdown_requested_isstd::atomic<bool>(line 28)io_threads_andwork_threads_arestd::vector<std::thread>(lines 29-30)The code at line 98 correctly passes these members to the
OnlineWebsocketServerconstructor.sherpa-onnx/csrc/CMakeLists.txt (3)
309-314: LGTM: WebSocket sources conditionally added to core.The WebSocket implementation files are appropriately added to the core library only when
SHERPA_ONNX_ENABLE_WEBSOCKETis enabled. This follows the existing pattern for other optional features (TTS, speaker diarization, etc.).
730-733: LGTM: Executable now relies on core library for WebSocket implementation.The
sherpa-onnx-online-websocket-serverexecutable (line 730-732) now only includesonline-websocket-server.cc, with the implementation logic (online-websocket-server-impl.ccandonline-websocket-server-app.cc) moved intosherpa-onnx-core(lines 310-312). This properly separates the executable entry point from the library logic, enabling Python bindings to reuse the same implementation.
452-459: Verify if ASIO definitions should be PUBLIC by inspecting public header files.The review correctly identifies a potential issue: compile definitions
ASIO_STANDALONEand_WEBSOCKETPP_CPP11_STL_are markedPRIVATE, which is correct only if no public headers insherpa-onnx-coreinclude asio or websocketpp headers.Verify whether headers like
online-websocket-server.hor other public API headers insherpa-onnx/csrc/include/directly includeasio.hppor websocketpp headers. If they do, change the definitions toPUBLIC:target_compile_definitions(sherpa-onnx-core PUBLIC ASIO_STANDALONE) target_compile_definitions(sherpa-onnx-core PUBLIC _WEBSOCKETPP_CPP11_STL_)
| )"; | ||
|
|
||
| // Global pointer for signal handling | ||
| static OnlineWebsocketServerApp *g_server_app = nullptr; |
There was a problem hiding this comment.
Critical: Dangling pointer risk with early returns.
The global pointer g_server_app is set in the constructor (line 51) but only cleared at the end of Run() (line 127). If Run() exits early (lines 81 or 89), the pointer remains set and will dangle when the OnlineWebsocketServerApp object is destroyed. A subsequent signal could trigger a use-after-free crash.
🔎 Proposed fix
Clear the global pointer in early return paths:
if (argc_ == 1) {
po.PrintUsage();
+ g_server_app = nullptr;
return;
}
po.Read(argc_, argv_);
if (po.NumArgs() != 0) {
SHERPA_ONNX_LOGE("Unrecognized positional arguments!");
po.PrintUsage();
+ g_server_app = nullptr;
return;
}Better yet, use RAII to manage the global pointer lifecycle. Consider adding a destructor:
OnlineWebsocketServerApp::~OnlineWebsocketServerApp() {
if (g_server_app == this) {
g_server_app = nullptr;
}
}Also applies to: 49-52, 127-127
🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/online-websocket-server-app.cc at line 40, The global
pointer g_server_app is assigned in OnlineWebsocketServerApp's constructor but
only cleared at the end of Run(), causing a dangling pointer if Run() returns
early; fix by ensuring g_server_app is cleared on all exit paths (e.g., set
g_server_app = nullptr before each early return in Run() at the exits currently
at lines like 81 and 89) and/or implement RAII by adding
OnlineWebsocketServerApp::~OnlineWebsocketServerApp() that clears g_server_app
when this == g_server_app so the global is always nulled when the object is
destroyed.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
git ls-files | grep -i "online-websocket-server-app.cc"Repository: k2-fsa/sherpa-onnx
Length of output: 180
🏁 Script executed:
# Read the relevant sections of the file
cat -n sherpa-onnx/csrc/online-websocket-server-app.cc | head -100Repository: k2-fsa/sherpa-onnx
Length of output: 3401
🏁 Script executed:
# Search for OnlineWebsocketServerApp instantiations and constructors
rg "OnlineWebsocketServerApp\s*\(" sherpa-onnx/csrc/online-websocket-server-app.cc -A 3Repository: k2-fsa/sherpa-onnx
Length of output: 198
🏁 Script executed:
# Check for signal handler and g_server_app usage
rg "g_server_app|signal|SIGINT|SIGTERM" sherpa-onnx/csrc/online-websocket-server-app.cc -B 2 -A 2Repository: k2-fsa/sherpa-onnx
Length of output: 819
🏁 Script executed:
# Search for any mutex or synchronization primitives in the file
rg "mutex|atomic|lock|thread|concurrent" sherpa-onnx/csrc/online-websocket-server-app.ccRepository: k2-fsa/sherpa-onnx
Length of output: 822
🏁 Script executed:
# Find the destructor
rg "~OnlineWebsocketServerApp|g_server_app\s*=\s*nullptr" sherpa-onnx/csrc/online-websocket-server-app.cc -B 2 -A 2Repository: k2-fsa/sherpa-onnx
Length of output: 239
🏁 Script executed:
# Check if there's a main function or StartServer in this file
rg "main|StartServer" sherpa-onnx/csrc/online-websocket-server-app.cc -B 1 -A 5Repository: k2-fsa/sherpa-onnx
Length of output: 418
🏁 Script executed:
# Find the header file to understand the class definition
git ls-files | grep -i "online-websocket-server.h$"Repository: k2-fsa/sherpa-onnx
Length of output: 104
🏁 Script executed:
# Read the full destructor and Stop() method
cat -n sherpa-onnx/csrc/online-websocket-server-app.cc | tail -50Repository: k2-fsa/sherpa-onnx
Length of output: 1638
The global g_server_app pointer lacks synchronization and is undocumented.
The assignment g_server_app = this (line 51) and subsequent pointer read in SignalHandler (lines 44-45) have no synchronization. While StartServer() ensures only one instance exists, this constraint is neither documented nor enforced—relying entirely on caller discipline. Additionally, the code demonstrates awareness of atomicity needs in Stop() (line 131 uses .exchange()), making the lack of synchronization on g_server_app inconsistent.
Add a comment above the global declaration documenting why a single instance is required, or consider using std::atomic or a static guard to enforce the single-instance requirement.
🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/online-websocket-server-app.cc at line 40, The global
g_server_app pointer is accessed without synchronization (assigned in
StartServer and read in SignalHandler) and lacks documentation about the
single-instance requirement; update the code to either make g_server_app a
std::atomic<OnlineWebsocketServerApp*> or add a static guard to enforce
single-instance creation, and add a comment explaining why only one instance is
allowed; specifically, change the global declaration of g_server_app to an
atomic pointer (or add a creation guard in StartServer) and ensure SignalHandler
reads it atomically, and add a brief comment above g_server_app referencing
StartServer, SignalHandler, and Stop (which already uses .exchange()) to explain
the concurrency model.
| static void SignalHandler(int32_t signal) { | ||
| SHERPA_ONNX_LOGE("Caught signal %d, stopping server...", signal); | ||
| if (g_server_app != nullptr) { | ||
| g_server_app->Stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Major: Signal handler calls non-async-signal-safe functions.
The SignalHandler calls SHERPA_ONNX_LOGE (a logging macro, likely using I/O) and Stop() (which also logs and manipulates ASIO contexts). POSIX signal handlers are severely restricted and may only call async-signal-safe functions. Calling unsafe functions like I/O, memory allocation, or mutex operations can cause undefined behavior, deadlocks, or crashes.
🔎 Recommended approach
Use a self-pipe or std::atomic flag pattern:
+static std::atomic<bool> g_shutdown_requested{false};
+
static void SignalHandler(int32_t signal) {
- SHERPA_ONNX_LOGE("Caught signal %d, stopping server...", signal);
- if (g_server_app != nullptr) {
- g_server_app->Stop();
- }
+ g_shutdown_requested.store(true, std::memory_order_release);
}Then in Run(), periodically check the flag or use ASIO signal handling:
asio::signal_set signals(io_conn_, SIGINT, SIGTERM);
signals.async_wait([this](auto, auto) { Stop(); });🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/online-websocket-server-app.cc around lines 42 - 47,
SignalHandler currently calls non-async-signal-safe functions (SHERPA_ONNX_LOGE
and g_server_app->Stop()), which is unsafe; replace that by using an async-safe
mechanism: either (preferred) remove usage of SignalHandler and register an
asio::signal_set in Run() (e.g., asio::signal_set signals(io_conn_, SIGINT,
SIGTERM); signals.async_wait([this](auto, auto){ Stop(); });) or make
SignalHandler only set an atomic flag (e.g., std::atomic<bool>
g_shutdown_requested) or write a byte to a self-pipe, and change Run()/the IO
loop to poll that flag or pipe and call Stop() from the main thread; ensure you
stop calling SHERPA_ONNX_LOGE or g_server_app->Stop() from inside SignalHandler
itself.
| void StartServer(int32_t argc, char *argv[]) { | ||
| OnlineWebsocketServerApp app(argc, argv); | ||
| app.Run(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suggest essential refactor: Consider exception safety in StartServer.
The StartServer function creates a local OnlineWebsocketServerApp and calls Run(). If Run() throws an exception before clearing g_server_app (line 127), the pointer will dangle. Consider wrapping in try-catch or relying on a destructor to clear the global pointer (as suggested earlier).
Enhanced safety with RAII
If the destructor clears g_server_app (as suggested in the first comment), this becomes exception-safe automatically. Otherwise, add explicit handling:
void StartServer(int32_t argc, char *argv[]) {
OnlineWebsocketServerApp app(argc, argv);
+ try {
app.Run();
+ } catch (...) {
+ g_server_app = nullptr;
+ throw;
+ }
}🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/online-websocket-server-app.cc around lines 140 - 143,
StartServer currently constructs a local OnlineWebsocketServerApp and calls
Run(), which can leave g_server_app dangling if Run() throws; either (A) make
OnlineWebsocketServerApp's destructor clear g_server_app so RAII ensures safety
on exception, or (B) wrap the construction/Run() call in a try-catch that clears
g_server_app in the catch (and rethrows or handle), referencing StartServer,
OnlineWebsocketServerApp, Run, and g_server_app so the global pointer is always
reset even on exceptions.
There was a problem hiding this comment.
Pull request overview
This PR refactors the online WebSocket server implementation into a more modular structure with Python bindings and graceful shutdown support. The refactoring extracts server logic into a reusable OnlineWebsocketServerApp class and exposes it to Python via pybind11.
Key Changes:
- Extracted server logic from
online-websocket-server.ccinto a newOnlineWebsocketServerAppclass for better modularity and reusability - Added Python bindings (
start_online_websocket_server) to enable WebSocket server management from Python - Implemented signal handling (SIGINT/SIGTERM) for graceful shutdown
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
sherpa-onnx/csrc/online-websocket-server.h |
New header defining OnlineWebsocketServerApp class and StartServer function |
sherpa-onnx/csrc/online-websocket-server.cc |
Simplified to instantiate and run OnlineWebsocketServerApp |
sherpa-onnx/csrc/online-websocket-server-app.cc |
Core server implementation with signal handling and thread management |
sherpa-onnx/csrc/CMakeLists.txt |
Added WebSocket sources to core library and compile definitions |
sherpa-onnx/python/csrc/online-websocket-server-app.h |
Python binding header declaration |
sherpa-onnx/python/csrc/online-websocket-server-app.cc |
Python binding wrapper converting Python arguments to C-style argv |
sherpa-onnx/python/csrc/sherpa-onnx.cc |
Integrated WebSocket server bindings into main Python module |
sherpa-onnx/python/csrc/CMakeLists.txt |
Added WebSocket build configuration for Python module |
sherpa-onnx/python/sherpa_onnx/__init__.py |
Conditionally imports start_online_websocket_server when available |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int argc = static_cast<int>(args.size()); | ||
| std::vector<std::string> args_str; | ||
| std::vector<char *> argv; | ||
|
|
||
| args_str.reserve(argc); | ||
| for (const auto &arg : args) { | ||
| args_str.push_back(arg.cast<std::string>()); | ||
| } | ||
|
|
||
| argv.reserve(argc + 1); | ||
| for (auto &str : args_str) { | ||
| argv.push_back(str.data()); | ||
| } | ||
| argv.push_back(nullptr); | ||
|
|
||
| StartServer(argc, argv.data()); |
There was a problem hiding this comment.
The StartServerWrapper function doesn't handle potential exceptions that might be thrown during the cast operation (line 21) or during StartServer execution. If an invalid argument type is passed in the args list, the cast will throw a pybind11::cast_error. Consider adding error handling to provide more informative error messages to Python users, or let pybind11's automatic exception translation handle it (which is the current behavior).
| int argc = static_cast<int>(args.size()); | |
| std::vector<std::string> args_str; | |
| std::vector<char *> argv; | |
| args_str.reserve(argc); | |
| for (const auto &arg : args) { | |
| args_str.push_back(arg.cast<std::string>()); | |
| } | |
| argv.reserve(argc + 1); | |
| for (auto &str : args_str) { | |
| argv.push_back(str.data()); | |
| } | |
| argv.push_back(nullptr); | |
| StartServer(argc, argv.data()); | |
| try { | |
| int argc = static_cast<int>(args.size()); | |
| std::vector<std::string> args_str; | |
| std::vector<char *> argv; | |
| args_str.reserve(argc); | |
| for (const auto &arg : args) { | |
| args_str.push_back(arg.cast<std::string>()); | |
| } | |
| argv.reserve(argc + 1); | |
| for (auto &str : args_str) { | |
| argv.push_back(str.data()); | |
| } | |
| argv.push_back(nullptr); | |
| StartServer(argc, argv.data()); | |
| } catch (const py::cast_error &e) { | |
| throw py::value_error( | |
| std::string("Invalid argument type in 'start_online_websocket_server' " | |
| "args: ") + | |
| e.what()); | |
| } catch (const std::exception &e) { | |
| throw py::runtime_error( | |
| std::string("Failed to start online websocket server: ") + e.what()); | |
| } |
| static void SignalHandler(int32_t signal) { | ||
| SHERPA_ONNX_LOGE("Caught signal %d, stopping server...", signal); | ||
| if (g_server_app != nullptr) { | ||
| g_server_app->Stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The signal handler uses functions that are not signal-safe. According to POSIX standards, signal handlers should only call async-signal-safe functions. Using SHERPA_ONNX_LOGE (which likely calls fprintf/printf) and calling Stop() (which uses std::atomic::exchange and asio operations) within a signal handler can lead to undefined behavior, including deadlocks or crashes.
Consider using a simpler approach: set a flag using std::atomic operations only (e.g., std::atomic_flag or std::atomic with memory_order_relaxed), and check this flag in the main event loop to trigger graceful shutdown.
| "Thread pool size for network connections."); | ||
|
|
||
| po.Register("num-work-threads", &num_work_threads, | ||
| "Thread pool size for for neural network " |
There was a problem hiding this comment.
There's a duplicate "for" in the error message text. It should read "Thread pool size for neural network" instead of "Thread pool size for for neural network".
| "Thread pool size for for neural network " | |
| "Thread pool size for neural network " |
|
|
||
| argv.reserve(argc + 1); | ||
| for (auto &str : args_str) { | ||
| argv.push_back(str.data()); |
There was a problem hiding this comment.
Using std::string::data() to obtain char* pointers is problematic because these pointers become invalid once the args_str vector goes out of scope or is modified. While StartServer is called immediately (line 30), if StartServer stores these pointers for later use rather than immediately copying the strings, this will lead to undefined behavior as the pointers will be dangling after StartServerWrapper returns.
The safer approach is to use c_str() instead of data(), or better yet, verify that StartServer immediately copies these strings and doesn't store the pointers for later use.
| argv.push_back(str.data()); | |
| argv.push_back(const_cast<char *>(str.c_str())); |
| // Register signal handlers | ||
| std::signal(SIGINT, SignalHandler); | ||
| std::signal(SIGTERM, SignalHandler); |
There was a problem hiding this comment.
The signal handlers registered in Run() are never explicitly cleaned up or reset. If Run() is called multiple times (e.g., if StartServer is called multiple times from Python), the signal handlers will still point to the global g_server_app pointer, which may become stale. Consider either: (1) resetting the signal handlers before returning from Run(), or (2) documenting that Run() should only be called once per process, or (3) ensuring the signal handlers check if g_server_app is still valid.
| void PybindOnlineWebsocketServerApp(py::module *m) { | ||
| m->def("start_online_websocket_server", &StartServerWrapper, | ||
| py::arg("args"), | ||
| "Start the online websocket server with command line arguments"); |
There was a problem hiding this comment.
The start_online_websocket_server function is blocking and will not return until the server is stopped. This means Python code calling this function will hang until a signal is received. Consider documenting this behavior clearly, or providing an alternative asynchronous API that allows Python code to start the server in a background thread and provides a way to stop it programmatically from Python (not just via signals).
| "Start the online websocket server with command line arguments"); | |
| "Start the online websocket server with command line arguments.\n\n" | |
| "This function is blocking and will not return until the server is " | |
| "stopped. If you need non-blocking behavior from Python, run it in " | |
| "a dedicated thread or process and manage its lifecycle there."); |
| OnlineWebsocketServerApp::OnlineWebsocketServerApp(int32_t argc, char *argv[]) | ||
| : argc_(argc), argv_(argv) { | ||
| g_server_app = this; | ||
| } |
There was a problem hiding this comment.
The global pointer g_server_app is set in the constructor and cleared in Run(), but this is not thread-safe if multiple instances could be created concurrently. While this may be unlikely in practice, it creates a potential race condition. Additionally, if Run() throws an exception before reaching line 127, g_server_app will remain pointing to an invalid object after the OnlineWebsocketServerApp instance is destroyed.
Consider either: (1) documenting that only one instance should exist at a time, or (2) using std::atomic<OnlineWebsocketServerApp*> for g_server_app, or (3) using a different approach for signal handling that doesn't rely on a global pointer.
| class OnlineWebsocketServerApp { | ||
| public: | ||
| OnlineWebsocketServerApp(int32_t argc, char *argv[]); | ||
| void Run(); | ||
| void Stop(); | ||
|
|
||
| private: | ||
| int32_t argc_; | ||
| char **argv_; | ||
| asio::io_context io_conn_; // ASIO context for connections | ||
| asio::io_context io_work_; // ASIO context for work | ||
| std::atomic<bool> shutdown_requested_{false}; | ||
| std::vector<std::thread> io_threads_; | ||
| std::vector<std::thread> work_threads_; | ||
| }; |
There was a problem hiding this comment.
The OnlineWebsocketServerApp class is declared without a namespace, placing it in the global namespace, while other components of the project appear to use the sherpa_onnx namespace. This is inconsistent with the project's code organization. Consider moving this class into the sherpa_onnx namespace for consistency and to avoid potential naming conflicts.
| void OnlineWebsocketServerApp::Stop() { | ||
| if (shutdown_requested_.exchange(true)) { | ||
| return; // Already requested | ||
| } | ||
|
|
||
| SHERPA_ONNX_LOGE("Stopping server..."); | ||
| io_conn_.stop(); | ||
| io_work_.stop(); | ||
| } |
There was a problem hiding this comment.
The Stop() method calls io_conn_.stop() and io_work_.stop() which will cause all threads running io_context::run() to exit, but there's no guarantee that the threads will have stopped by the time Stop() returns. The main thread in Run() will continue to call join() on all threads, but there's no coordination between the signal handler calling Stop() and the main thread. This could potentially cause issues if the signal handler returns while threads are still being joined. Consider adding proper synchronization or documenting the expected behavior.
| # Optional websocket server (only available if built with SHERPA_ONNX_ENABLE_WEBSOCKET) | ||
| try: | ||
| from sherpa_onnx.lib._sherpa_onnx import start_online_websocket_server | ||
| except ImportError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except ImportError: | |
| except ImportError: | |
| # Websocket server support is optional; ignore if the symbol is not available. |
| PybindVersion(&m); | ||
|
|
||
| #if SHERPA_ONNX_ENABLE_WEBSOCKET == 1 | ||
| PybindOnlineWebsocketServerApp(&m); |
There was a problem hiding this comment.
Please follow
https://github.com/manickavela29/sherpa-onnx/blob/e7d617d14fbc81f9665d58e89e2e69cd727409ef/sherpa-onnx/python/csrc/sherpa-onnx.cc#L107
to add a dummy implementation
when websocket is not enabled.
| from .online_recognizer import OnlineRecognizer | ||
| from .utils import text2token | ||
|
|
||
| # Optional websocket server (only available if built with SHERPA_ONNX_ENABLE_WEBSOCKET) |
There was a problem hiding this comment.
You can remove it after you add a dummy implementation for it in C++.
|
|
||
| namespace sherpa_onnx { | ||
|
|
||
| static void StartServerWrapper(py::list args) { |
There was a problem hiding this comment.
Can you use a py::dict?
A dict, instead of a list, can simplify user's life since they can use keyword arguments when calling this function.
| @@ -306,6 +306,13 @@ if(SHERPA_ONNX_ENABLE_CHECK) | |||
| list(APPEND sources log.cc) | |||
There was a problem hiding this comment.
Can you add a python example to show how to use this pull request?
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the online websocket server into a separate application class, which improves modularity and enables Python bindings. The changes are generally good, but I've identified a critical issue with signal handling that could lead to undefined behavior. I've also found a const-correctness issue in the argument passing from Python to C++ and some code duplication in the CMake files. My comments provide detailed suggestions for addressing these points to improve the robustness and maintainability of the code.
| // Global pointer for signal handling | ||
| static OnlineWebsocketServerApp *g_server_app = nullptr; | ||
|
|
||
| static void SignalHandler(int32_t signal) { | ||
| SHERPA_ONNX_LOGE("Caught signal %d, stopping server...", signal); | ||
| if (g_server_app != nullptr) { | ||
| g_server_app->Stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current signal handling mechanism using a global pointer (g_server_app) and a C-style signal handler is not safe. The SignalHandler calls SHERPA_ONNX_LOGE and g_server_app->Stop(), which are not async-signal-safe. This can lead to deadlocks, data corruption, or other undefined behavior.
A much safer and cleaner approach is to use asio::signal_set to handle signals within the Asio event loop. This avoids global state and ensures the stop logic is executed safely.
Please consider the following refactoring:
- Remove the global
g_server_apppointer and theSignalHandlerfunction (lines 39-47). - Remove the assignment
g_server_app = this;from the constructor (line 51). - In the
Run()method, replacestd::signal()calls (lines 95-96) withasio::signal_set:asio::signal_set signals(io_conn_, SIGINT, SIGTERM); signals.async_wait( [this](const asio::error_code & /*error*/, int32_t signal_number) { SHERPA_ONNX_LOGE("Caught signal %d, stopping server...", signal_number); Stop(); });
- Remove
g_server_app = nullptr;at the end ofRun()(line 127).
| std::vector<std::string> args_str; | ||
| std::vector<char *> argv; | ||
|
|
||
| args_str.reserve(argc); | ||
| for (const auto &arg : args) { | ||
| args_str.push_back(arg.cast<std::string>()); | ||
| } | ||
|
|
||
| argv.reserve(argc + 1); | ||
| for (auto &str : args_str) { | ||
| argv.push_back(str.data()); | ||
| } | ||
| argv.push_back(nullptr); |
There was a problem hiding this comment.
There's a const-correctness issue here and in the related C++ app code. std::string::data() on a non-const string returns a char*, but these arguments are not meant to be modified. It's safer to use const char*.
The argv vector should be std::vector<const char*>, populated using c_str(). After this change, you will need to update StartServer and OnlineWebsocketServerApp to accept const char*[] to fix the resulting compilation error and complete the const-correctness refactoring. This is highly recommended for type safety.
| std::vector<std::string> args_str; | |
| std::vector<char *> argv; | |
| args_str.reserve(argc); | |
| for (const auto &arg : args) { | |
| args_str.push_back(arg.cast<std::string>()); | |
| } | |
| argv.reserve(argc + 1); | |
| for (auto &str : args_str) { | |
| argv.push_back(str.data()); | |
| } | |
| argv.push_back(nullptr); | |
| std::vector<std::string> args_str; | |
| std::vector<const char *> argv; | |
| args_str.reserve(argc); | |
| for (const auto &arg : args) { | |
| args_str.push_back(arg.cast<std::string>()); | |
| } | |
| argv.reserve(argc + 1); | |
| for (const auto &str : args_str) { | |
| argv.push_back(str.c_str()); | |
| } | |
| argv.push_back(nullptr); |
| if(SHERPA_ONNX_ENABLE_WEBSOCKET) | ||
| target_compile_definitions(sherpa-onnx-core PRIVATE ASIO_STANDALONE) | ||
| target_compile_definitions(sherpa-onnx-core PRIVATE _WEBSOCKETPP_CPP11_STL_) | ||
| target_compile_definitions(sherpa-onnx-core PUBLIC SHERPA_ONNX_ENABLE_WEBSOCKET=1) | ||
| if(NOT WIN32) | ||
| target_compile_options(sherpa-onnx-core PRIVATE -Wno-deprecated-declarations) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
These WebSocket-related compile definitions and options are duplicated in sherpa-onnx/python/csrc/CMakeLists.txt. To avoid duplication and improve maintainability, you could create an INTERFACE library that holds these settings.
For example, you could add this to a common CMake file:
add_library(sherpa-onnx-websocket-interface INTERFACE)
if(SHERPA_ONNX_ENABLE_WEBSOCKET)
target_compile_definitions(sherpa-onnx-websocket-interface INTERFACE
ASIO_STANDALONE
_WEBSOCKETPP_CPP11_STL_
SHERPA_ONNX_ENABLE_WEBSOCKET=1
)
if(NOT WIN32)
target_compile_options(sherpa-onnx-websocket-interface INTERFACE -Wno-deprecated-declarations)
endif()
endif()Then, you can link sherpa-onnx-core and _sherpa_onnx against this interface library, which will propagate the settings.
| if(SHERPA_ONNX_ENABLE_WEBSOCKET) | ||
| target_compile_definitions(_sherpa_onnx PRIVATE ASIO_STANDALONE) | ||
| target_compile_definitions(_sherpa_onnx PRIVATE _WEBSOCKETPP_CPP11_STL_) | ||
| target_compile_definitions(_sherpa_onnx PRIVATE SHERPA_ONNX_ENABLE_WEBSOCKET=1) | ||
| if(NOT WIN32) | ||
| target_compile_options(_sherpa_onnx PRIVATE -Wno-deprecated-declarations) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
These WebSocket-related compile definitions and options are duplicated from sherpa-onnx/csrc/CMakeLists.txt. To avoid duplication and improve maintainability, consider creating a shared INTERFACE library for these settings and linking both sherpa-onnx-core and _sherpa_onnx against it.
Hi @csukuangfj ,
I have refactored the previous implementation,
Below code base is functional tested and cleaned up
I think you can suggest more for naming for more clarity
or clean up.
overall summary,
-- refactoring online-websocket-server
-- pybind interface
-- signal termination, graceful shut down
Thanks
closed #1931
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.