fix: create env on append in /update handler#1373
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures that calling the /update endpoint with update='append' in a previously unused environment correctly initializes that environment and creates the target window instead of failing silently. Sequence diagram for /update append creating environment on first usesequenceDiagram
actor Client
participant Server
participant HandlerState
Client->>Server: POST /update(env=new_env, win=test, update=append)
Server->>Server: update
Server->>Server: extract_eid
alt eid not in handler.state
Server->>HandlerState: handler.state[eid] = {jsons: {}, reload: {}}
end
alt win not in handler.state[eid].jsons
Server->>HandlerState: register_window
else win exists
Server->>HandlerState: append_to_window
end
Server-->>Client: response with created_or_updated_window
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider factoring out the
handler.state[eid] = {"jsons": {}, "reload": {}}initialization into a shared helper or using the same code path as other env-creation logic to avoid duplicating the default structure and keep behavior consistent if it changes in the future. - If
handler.statecan be accessed concurrently (e.g., multiple requests for the sameeid), you may want to guard theif eid not in handler.stateblock or use a safer pattern (e.g.,setdefault) to avoid potential race conditions or partial initialization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider factoring out the `handler.state[eid] = {"jsons": {}, "reload": {}}` initialization into a shared helper or using the same code path as other env-creation logic to avoid duplicating the default structure and keep behavior consistent if it changes in the future.
- If `handler.state` can be accessed concurrently (e.g., multiple requests for the same `eid`), you may want to guard the `if eid not in handler.state` block or use a safer pattern (e.g., `setdefault`) to avoid potential race conditions or partial initialization.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds initialization of per-environment state in the web update handler to avoid missing-key errors when handling requests for a new eid.
Changes:
- Initialize
handler.state[eid]when aneidis first encountered. - Ensures
jsonsandreloaddictionaries exist before accessing window state.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rajnisht7
left a comment
There was a problem hiding this comment.
@tonypzy tested the reproduction steps from the original issue, but worked fine appending to a window in a new env successfully created both env and the window, and returned the expected window id. can you share code/steps to reproduce it
Try this. |
|
@tonypzy is this problem limited to direct |
It is not limited to direct |
|
@tonypzy and ensured that both envs |
This PR fixes the path where the client is bypassed and an |
There was a problem hiding this comment.
@tonypzy as per the ci logs
Checking for scripts.
INFO:root:Server started
INFO:root:Application Started
INFO:root:Working directory: /home/acer/.visdom
You can navigate to http://localhost:8097
INFO:tornado.access:200 POST /env/main (::1) 0.37ms
INFO:tornado.access:101 GET /vis_socket (::1) 0.34ms
INFO:root:Opened visdom source socket from ip: ::1
INFO:tornado.access:200 POST /win_exists (::1) 238.08ms
INFO:tornado.access:200 POST /win_exists (::1) 0.24ms
INFO:tornado.access:200 POST /win_exists (::1) 0.15ms
INFO:tornado.access:200 POST /events (::1) 0.21ms
whenever we pass update append for non-existent wins or envs python automatically hits /win_exists first and routes to /events
also the endpoint this PR address is never meant to be called directly nor we recommend to use it that way
that is
requests.post("http://localhost:8097/update", data=json.dumps({
"win": "test",
"eid": "main2", # non-existent env
"append": True,
...
}))
even though the fix is harmless but it addresses an endpoint that no real user calls directly
can you share a concrete real world scenario where someone would bypass the client and hit /update with a non-existent env?
Actually, lines 534 and 599 in Regarding the practical application, there is a race condition during concurrent execution. If an |
Description
Motivation and Context
Fix #775
When
/updateis called withappend=Truefor a not-yet-created environment, the handler crashes withKeyError(HTTP 500) athandler.state[eid]["jsons"]because it assumes the env already exists.By adding:
register_windowcreates the window and broadcasts the env list as usual.How Has This Been Tested?
Direct HTTP POST to
/updatewith a non-existent environment.Screenshots (if appropriate):
N/A
Types of changes
Checklist:
py/visdom/VERSIONaccording to Semantic VersioningSummary by Sourcery
Bug Fixes: