Implement Auto-Shutdown When Browser Closes (solves Issue #151)#166
Implement Auto-Shutdown When Browser Closes (solves Issue #151)#166Ayushmaan06 wants to merge 4 commits intoOpenMS:mainfrom
Conversation
WalkthroughThis change introduces a global session management system within the Streamlit application. It adds global variables to track active sessions and last heartbeat timestamps. Functions are defined to increment and decrement session counts and invoke server shutdown when no sessions remain. Tornado web handlers are added to process heartbeat pings and handle session closures. A separate Tornado server thread along with a heartbeat monitor thread ensures inactivity is detected, while client-side JavaScript sends periodic heartbeat pings and manages tab closure events. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Browser (JS)
participant Tornado as Tornado Server
participant Session as Session Manager
participant Monitor as Heartbeat Monitor
Client->>Tornado: Sends heartbeat ping every 3s
Tornado->>Session: Update last_heartbeat timestamp
Client->>Tornado: Sends tab close event on exit
Tornado->>Session: Decrement active session count
Monitor->>Session: Check last_heartbeat & active sessions
Session-->>Tornado: Trigger shutdown if no active sessions and inactive heartbeat
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app.py (7)
11-11: Unused import flagged by static analysis.Since
pyopenmsmay be unused, consider removing it to keep the code clean. However, if it’s needed for side effects or platform-specific fixes (as the comment suggests), feel free to ignore this suggestion:-import pyopenms # required import for Windows🧰 Tools
🪛 Ruff (0.8.2)
11-11:
pyopenmsimported but unusedRemove unused import:
pyopenms(F401)
15-16: Consider adding error handling when loading settings.Wrap file operations in a try-except block to handle missing or invalid JSON content gracefully.
try: with open("settings.json", "r") as f: st.session_state.settings = json.load(f) except (FileNotFoundError, json.JSONDecodeError) as e: st.session_state.settings = {} print(f"Warning: Failed to load settings.json: {e}")
19-20: Watch out for concurrent write access to global variables.If you anticipate multiple concurrent requests, consider using a lock or synchronization mechanism for
global_active_sessionsandlast_heartbeatto avoid race conditions.
22-25: Increment logic is simple but may require synchronization.Add a threading lock if concurrency becomes relevant:
lock = threading.Lock() def increment_active_sessions(): global global_active_sessions + with lock: global_active_sessions += 1
27-33: Ensure no race conditions when decrementing sessions.If multiple requests close quickly, you could trigger a double decrement. A lock can prevent the session count from dropping below zero inadvertently.
34-36: Graceful shutdown considerations.Depending on user expectations, you might allow a cleanup procedure instead of sending SIGTERM immediately, ensuring any pending tasks finish before stopping the server.
45-51: Blocking call in the POST request.Using
time.sleep(1)in a Tornado handler can impact responsiveness. Consider an async approach or removing the delay if it’s not strictly needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
11-11: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
🔇 Additional comments (11)
app.py (11)
2-2: No concerns with usingstreamlit.components.v1.
5-9: Imports appear correct for new functionality.
13-13: No issues with the comment line.
38-42: Session registration could overcount if a user opens multiple tabs.Confirm whether each new tab should represent a unique session. If not, consider using a dedicated session ID to track real user sessions accurately.
52-58: Heartbeat handler logic looks straightforward.
59-67: Potential port conflict when listening on 8501.If Streamlit already binds to 8501, confirm that Tornado can coexist. Otherwise, use a separate port (e.g., 8502) or an unused one to avoid conflicts.
68-68: Running the Tornado server in a daemon thread is acceptable.
70-81: Heartbeat monitor is implemented correctly.Though some tasks might require a lock around the
last_heartbeatcheck, the current approach is sufficient for lightweight usage.
84-100: Regular heartbeat injection looks solid.
101-112: Close-tab script is an effective fallback.
139-144: Injection order of scripts afterpg.run()is valid.
|
@Ayushmaan06 thanks for looking into it. |
| import time | ||
| import threading | ||
| import signal | ||
| import tornado.web |
There was a problem hiding this comment.
app.py
Outdated
| (r"/heartbeat", HeartbeatHandler) | ||
| ] | ||
| app = tornado.web.Application(routes) | ||
| app.listen(port=8501) # Adjust if needed; matches Streamlit port. |
There was a problem hiding this comment.
app.py
Outdated
| pg.run() | ||
|
|
||
| # Inject the heartbeat script first so the server sees regular pings. | ||
| insert_heartbeat_script() |
There was a problem hiding this comment.
make sure for now just effect local.
app.py
Outdated
|
|
||
| def increment_active_sessions(): | ||
| global global_active_sessions | ||
| global_active_sessions += 1 |
There was a problem hiding this comment.
I have no idea of how python does this, but in C++ this would be a race condition.
Not sure if this is even multithreaded code but Copilot suggests
import threading
# --- Global Session Counter with Lock ---
global_active_sessions = 0
lock = threading.Lock()
def increment_active_sessions():
global global_active_sessions
with lock:
global_active_sessions += 1
etc... (for all mentions of the global variable)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
app.py (3)
18-21:⚠️ Potential issueAddress potential race conditions with global variables.
The global variables for session counting and heartbeat tracking are accessed from multiple threads without synchronization, which could lead to race conditions.
Implement thread synchronization using locks:
# --- Global Session Counter --- global_active_sessions = 0 last_heartbeat = 0 # Global heartbeat timestamp +session_lock = threading.Lock() # Lock for thread safety
27-30:⚠️ Potential issueMake increment_active_sessions thread-safe.
This function directly modifies the global counter without synchronization, which could cause inaccurate counting in a multithreaded environment.
Update to use thread synchronization:
def increment_active_sessions(): global global_active_sessions - global_active_sessions += 1 + with session_lock: + global_active_sessions += 1 print(f"Session connected, total active sessions: {global_active_sessions}")
32-37:⚠️ Potential issueMake decrement_active_sessions thread-safe.
This function also directly modifies the global counter without synchronization and makes a critical decision to shut down based on the counter value.
Update to use thread synchronization:
def decrement_active_sessions(): global global_active_sessions - global_active_sessions -= 1 - print(f"Session disconnected, total active sessions: {global_active_sessions}") - if global_active_sessions <= 0: + with session_lock: + global_active_sessions -= 1 + current_count = global_active_sessions + print(f"Session disconnected, total active sessions: {current_count}") + if current_count <= 0: shutdown()
🧹 Nitpick comments (4)
requirements.txt (1)
11-11: Dependency addition looks good.The addition of Tornado is appropriate for implementing the heartbeat and close app endpoints needed for the auto-shutdown functionality.
Consider pinning the version (e.g.,
tornado==6.2.0) for better reproducibility of builds, especially since other dependencies are pinned.app.py (3)
11-11: Clarify the reason for the unused import.The
pyopenmsimport is flagged as unused by the static analyzer, but there's a comment stating it's required for Windows.Improve the comment and suppress the linter warning:
-import pyopenms # required import for Windows +import pyopenms # noqa: F401 - Required import for Windows to initialize OpenMS library🧰 Tools
🪛 Ruff (0.8.2)
11-11:
pyopenmsimported but unusedRemove unused import:
pyopenms(F401)
50-56: Add request validation to prevent unauthorized shutdowns.The current implementation allows any client to trigger the closeapp endpoint, which could lead to denial of service if exploited.
Add basic validation to ensure requests are legitimate:
class CloseAppHandler(tornado.web.RequestHandler): def post(self): + # Validate the request comes from the expected origin + origin = self.request.headers.get('Origin', '') + if not origin.startswith('http://localhost:') and not origin.startswith('https://localhost:'): + self.set_status(403) + self.write("Forbidden") + return time.sleep(1) decrement_active_sessions() self.write("OK")
36-41: Improve shutdown logging and add safety mechanism for session counter.The current implementation has minimal logging and no recovery mechanism if the session counter gets out of sync.
Enhance the shutdown function and add a recovery mechanism:
def decrement_active_sessions(): global global_active_sessions - global_active_sessions -= 1 - print(f"Session disconnected, total active sessions: {global_active_sessions}") - if global_active_sessions <= 0: + with session_lock: + global_active_sessions = max(0, global_active_sessions - 1) # Prevent negative values + current_count = global_active_sessions + print(f"Session disconnected, total active sessions: {current_count}") + if current_count <= 0: shutdown() def shutdown(): - print("🚨 No active users. Shutting down Streamlit server...") + import logging + logging.warning("🚨 No active users. Shutting down Streamlit server...") + print("🚨 No active users. Shutting down Streamlit server...") os.kill(os.getpid(), signal.SIGTERM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env(1 hunks)app.py(2 hunks)requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
11-11: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
🔇 Additional comments (2)
app.py (2)
64-71: Verify Tornado port configuration and add error handling.The Tornado server is configured to listen on port 8502, but there's no error handling if the port is already in use.
Ensure the port matches what's in the Streamlit
.tomlfile and add error handling:def start_tornado_server(): - routes = [ - (r"/_closeapp", CloseAppHandler), - (r"/heartbeat", HeartbeatHandler) - ] - app = tornado.web.Application(routes) - app.listen(port=8502) # Adjust if needed; matches Streamlit port. - tornado.ioloop.IOLoop.current().start() + try: + routes = [ + (r"/_closeapp", CloseAppHandler), + (r"/heartbeat", HeartbeatHandler) + ] + app = tornado.web.Application(routes) + app.listen(port=8502) # Adjust if needed; matches Streamlit port. + print("Tornado server started on port 8502") + tornado.ioloop.IOLoop.current().start() + except OSError as e: + print(f"Error starting Tornado server: {e}")Run this script to verify the port configuration in .streamlit/config.toml:
#!/bin/bash # Check if port 8502 is defined in .streamlit/config.toml if [ -f .streamlit/config.toml ]; then echo "Checking Streamlit port configuration..." if grep -q "port = 8502" .streamlit/config.toml; then echo "✓ Port 8502 is correctly configured in .streamlit/config.toml" else echo "Port configured in .streamlit/config.toml:" grep "port" .streamlit/config.toml || echo "No port configuration found" fi else echo "No .streamlit/config.toml file found" fi
133-135: Good use of environment variable to control local-only functionality.The conditional activation based on the LOCAL_RUN environment variable correctly addresses the requirement to have this functionality only apply to local executions.
Yes, the solution can accommodate long‐running processes. The heartbeat monitor ensures the app stays active as long as it receives regular pings, and if no heartbeat is detected within the timeout interval, it automatically shuts down. Moreover, the auto‑shutdown features (heartbeat pings and close‑tab beacon) are only activated if the app is run locally (controlled via the LOCAL_RUN environment variable), ensuring they don't interfere with production or other long‑running environments. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app.py (1)
87-93:⚠️ Potential issueHonor the “no sessions active” condition before shutdown.
Despite the comment “If no heartbeat received in 15 seconds and no sessions are active, shutdown,” the code ignoresactive. Update the condition to avoid forcing shutdown when sessions remain.- if elapsed > 15: - shutdown() + if elapsed > 15 and active <= 0: + shutdown()🧰 Tools
🪛 Ruff (0.8.2)
91-91: Local variable
activeis assigned to but never usedRemove assignment to unused variable
active(F841)
🧹 Nitpick comments (3)
app.py (3)
11-11: Consider removing or justifying the unusedpyopenmsimport.
Static analysis flagspyopenmsas unused. If it's only required for side effects on Windows, please leave a clarifying comment. Otherwise, consider removing it to keep imports tidy.- import pyopenms # required import for Windows + # If truly needed for side effects on Windows, keep and comment why. + # Otherwise, remove: + # import pyopenms🧰 Tools
🪛 Ruff (0.8.2)
11-11:
pyopenmsimported but unusedRemove unused import:
pyopenms(F401)
53-59: Question the one-second delay on browser unload.
Sleeping for one second (line 56) might not be necessary and can delay resource cleanup. Unless there's a specific reason, consider removing or reducing this delay.def post(self): - time.sleep(1) decrement_active_sessions() self.write("OK")
100-105: Avoid hardcoding the port in the heartbeat script.
If an alternative port is used or changed in configuration, the fetch URL becomes invalid. Usewindow.locationor a relative path for better flexibility.function sendHeartbeat() { - fetch('http://localhost:8502/heartbeat', {method: 'POST'}); + const baseUrl = window.location.protocol + '//' + window.location.host; + fetch(baseUrl + '/heartbeat', {method: 'POST'}); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
11-11: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
91-91: Local variable active is assigned to but never used
Remove assignment to unused variable active
(F841)
| # This handler is called on browser unload (if it fires) | ||
| class CloseAppHandler(tornado.web.RequestHandler): | ||
| def post(self): | ||
| time.sleep(1) | ||
| decrement_active_sessions() | ||
| self.write("OK") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add client-side usage of the /_closeapp endpoint.
The CloseAppHandler endpoint is defined but never invoked by the client script. Implementing a beforeunload or unload listener with navigator.sendBeacon('/_closeapp') ensures the server receives disconnection signals reliably.
<script>
function sendHeartbeat() {
fetch('http://localhost:8502/heartbeat', {method: 'POST'});
}
// ...
+ window.addEventListener('beforeunload', () => {
+ navigator.sendBeacon('http://localhost:8502/_closeapp');
+ });
</script>Also applies to: 70-71
|
@singjc where we able to implement something like this in massdash? |
We don't have an auto shutdown, but we have a button that the user can click to terminate the process and close the browser tab that contains the app. https://github.com/Roestlab/massdash/blob/5cb2af88ec79d873ab5bc0e790cbc209af6a41a7/massdash/util.py#L471-L500 |
|
@Ayushmaan06 thanks for looking into it. I run localy, and app is automatically closed even there is active tabs (open in browser)? can you look into it. |
The app closes automatically because my current logic checks if no active Streamlit tabs (i.e., no heartbeat signals) are present for 15 seconds, and if none are found, it calls “shutdown()” to terminate the process. That’s why the app stops even when a local tab might be open (if it isn’t sending heartbeats). The MassDash approach uses a more explicit “Close” button that manually triggers the shutdown, including attempts to close the browser tab via system keystrokes. If we prefer that behavior instead of an automatic timeout, we can adapt MassDash’s function to our code by adding a “Close” button in the Streamlit UI. Then, when a user clicks it, the app would close the tab (if permissions allow) and terminate the process, rather than waiting on the 15-second timer. @Arslan-Siraj Please, Let me know if you want me to integrate the MassDash flow or keep our current automatic shutdown! |
5 participants
Solves #151
This PR introduces an auto‑shutdown mechanism for the Streamlit server. Key changes include:
• Adding Tornado endpoints (/heartbeat and /_closeapp) that handle periodic heartbeat pings and browser unload events.
• Implementing a heartbeat monitor in a background thread that shuts down the server if no heartbeat is received for 10 seconds and no active sessions exist.
• Injecting client‑side JavaScript (using sendBeacon on window unload and a regular heartbeat script) to notify the server when the browser closes.
These changes ensure that the Streamlit process terminates automatically when the browser is closed, providing a better user experience, especially on Windows when deployed as an executable.
Summary by CodeRabbit
Summary by CodeRabbit