Improve how server/js client handle unexpected errors #6798

freddyaboulton · 2023-12-14T22:13:29Z

Description

Noticed two things when I was debugging flaky tests:

If there's an exception in the queue/data/ route the client will hang forever. the server now catches general exceptions
If the message didn't have an event_id, the client would error out, e.g. heartbeat, server_stopped. This wouldn't cause any noticeable errors to users (except console exceptions) but wanted to fix.My guess is that the playwright tests are mysteriously erroring server side so printing the exception message in the UI can help us debug.
gr.Info and gr.Warning would also raise exceptions in the client. Would not be obvious to users but not desirable anyways.
Think I found the issue for the weird HTTPExceptions in logs

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh
You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

gradio-pr-bot · 2023-12-14T22:13:44Z

🪼 branch checks and previews

•	Name	Status	URL
	Spaces	ready!	Spaces preview
	Website	ready!	Website preview
	Storybook	ready!	Storybook preview
	Visual tests	1 failing test	Build review
🦄	Changes	detected!	Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/6966114b9f36274e85c2196ab551de949e36a226/gradio-4.9.1-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@6966114b9f36274e85c2196ab551de949e36a226#subdirectory=client/python"

gradio-pr-bot · 2023-12-14T22:14:16Z

🦄 change detected

This Pull Request includes changes to the following packages.

Package	Version
`@gradio/client`	`patch`
`gradio`	`patch`

Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Improve how server/js client handle unexpected errors

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

Maintainers can change the version label to modify the version bump.
If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

freddyaboulton · 2023-12-15T17:37:42Z

client/js/src/client.ts

 											}
 										}
+									} catch (e) {
+										console.error("Unexpected client exception", e);


This should fix the HTTPException errors we keep seeing in the logs.

Explanation

https://www.loom.com/share/5f7abfdb2f2143c58982f11dab2538b2?sid=d176fd1c-b5f5-43f5-be5f-f72af9b3abea

Fix

https://www.loom.com/share/d6b9d87d05b64f69bfb8158ac565593f?sid=ca833ba1-4579-4536-8e14-50396ba3261c

freddyaboulton · 2023-12-15T17:38:41Z

Not sure how to test this actually. Should be good for review though!

abidlabs · 2023-12-15T18:18:39Z

Nice! cc @aliabid94

abidlabs · 2023-12-15T18:19:10Z

Is this expected to resolve #6713?

freddyaboulton · 2023-12-15T19:34:38Z

@abidlabs Hard to know for sure since we don't know how to repro those issues. However, I did just verify that exceptions in user functions don't break the queue/app

https://www.loom.com/share/0b7d99654b444b6a8ff6d0c320c878ed?sid=bb08ee79-eb06-48f0-822d-c89d0819a1db

aliabid94 · 2023-12-15T19:42:37Z

gradio/routes.py

-                except asyncio.CancelledError as e:
-                    del blocks._queue.pending_messages_per_session[session_hash]
-                    await blocks._queue.clean_events(session_hash=session_hash)
+                except Exception as e:


Has to be BaseException to catch everything - asyncio.CancelledError is not an instance of Exception

aliabid94 · 2023-12-15T20:05:28Z

client/js/src/client.ts

 								event_callbacks[event_id] = callback;
+								if (!stream_open) {


why did you move this?

Figured the stream should be opened after the callback got added

ok sure, but if a second request is sent while a first one is pending, the stream will be open from the first request, so we shouldn't be expecting it to be the case that the stream is receiving messages for an event only after post request is complete.
Therefore, it can technically happen that the event stream starts sending messages for an event before the POST request to upload data finishes, and therefore the event callback to handle the message is not ready, which is a race condition that I do not know if we are hitting. I think I do resolve this in a PR I'm working on though

aliabid94 · 2023-12-15T20:09:03Z

client/js/src/client.ts

@@ -1014,6 +1053,12 @@ export function api_factory(
 				event_stream = new EventSource(url);
 				event_stream.onmessage = async function (event) {
 					let _data = JSON.parse(event.data);
+					if (_data.session_hash) {
+						await Promise.all(
+							event_ids.map((event_id) => event_callbacks[event_id](_data))


If this is the only reason we're keeping track of event_ids, we already have that list in the keys of event_callbacks. No need to create a second list right?

also would prefer if the logic for targetting every event listener was more explicit than if the session_hash key is present. We don't use the session_hash value anyway. I think it'd make more sense that if an event_id is not provided, we trigger every event.

Also made the change about checking the absence of event_id to trigger all listeners!

aliabid94 · 2023-12-15T20:10:57Z

If there's an exception in the queue/data/ route the client will hang forever. the server now catches general exceptions

What exceptions were happening?

freddyaboulton · 2023-12-15T20:20:38Z

What exceptions were happening?

We don't know because the client would hang forever. Although I think it's the reason the playwright tests were just hanging forever in the cases they failed.

aliabid94

LGTM!

freddyaboulton · 2023-12-15T21:01:25Z

Thanks @aliabid94 !

freddyaboulton added the v: patch A change that requires a patch release label Dec 15, 2023

freddyaboulton commented Dec 15, 2023

View reviewed changes

freddyaboulton marked this pull request as ready for review December 15, 2023 18:00

freddyaboulton requested review from aliabid94, pngwn, abidlabs, whitphx, aliabd, dawoodkhan82 and hannahblair December 15, 2023 18:01

aliabid94 reviewed Dec 15, 2023

View reviewed changes

freddyaboulton and others added 9 commits December 15, 2023 15:51

Client fixes

158c9eb

fix

db3a77a

add changeset

1358438

commented out code

8ab3842

add changeset

d8c49e4

Log error give generic message

1bce43d

Add client side catch

e0dd906

remove exception 😂

8c98fb8

Add test

d57bf0e

freddyaboulton added 5 commits December 15, 2023 15:51

Fix info and warning

f878436

lint

bf9ac12

Use BaseException

4af681b

Use event_callbacks

566c189

Use event_id not present

6d0f576

freddyaboulton force-pushed the client-handle-server-failures branch from 20c5461 to 6d0f576 Compare December 15, 2023 20:51

aliabid94 approved these changes Dec 15, 2023

View reviewed changes

freddyaboulton merged commit 245d58e into main Dec 15, 2023
15 checks passed

freddyaboulton deleted the client-handle-server-failures branch December 15, 2023 21:01

pngwn mentioned this pull request Dec 15, 2023

chore: update versions #6797

Merged

freddyaboulton mentioned this pull request Dec 15, 2023

E2E test that Queue Full does not break the client. Also that backend exceptions don't break the client or queue. #6812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve how server/js client handle unexpected errors #6798

Improve how server/js client handle unexpected errors #6798

freddyaboulton commented Dec 14, 2023 •

edited

gradio-pr-bot commented Dec 14, 2023 •

edited

gradio-pr-bot commented Dec 14, 2023 •

edited

Something isn't right?

freddyaboulton Dec 15, 2023

freddyaboulton commented Dec 15, 2023

abidlabs commented Dec 15, 2023

abidlabs commented Dec 15, 2023

freddyaboulton commented Dec 15, 2023

aliabid94 Dec 15, 2023

aliabid94 Dec 15, 2023

freddyaboulton Dec 15, 2023

aliabid94 Dec 15, 2023

aliabid94 Dec 15, 2023

aliabid94 Dec 15, 2023

freddyaboulton Dec 15, 2023

freddyaboulton Dec 15, 2023

aliabid94 commented Dec 15, 2023

freddyaboulton commented Dec 15, 2023

aliabid94 left a comment

freddyaboulton commented Dec 15, 2023

Improve how server/js client handle unexpected errors #6798

Improve how server/js client handle unexpected errors #6798

Conversation

freddyaboulton commented Dec 14, 2023 • edited

Description

🎯 PRs Should Target Issues

Tests

gradio-pr-bot commented Dec 14, 2023 • edited

🪼 branch checks and previews

gradio-pr-bot commented Dec 14, 2023 • edited

🦄 change detected

This Pull Request includes changes to the following packages.

With the following changelog entry.

Something isn't right?

Choose a reason for hiding this comment

Explanation

Fix

freddyaboulton commented Dec 15, 2023

abidlabs commented Dec 15, 2023

abidlabs commented Dec 15, 2023

freddyaboulton commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabid94 commented Dec 15, 2023

freddyaboulton commented Dec 15, 2023

aliabid94 left a comment

Choose a reason for hiding this comment

freddyaboulton commented Dec 15, 2023

freddyaboulton commented Dec 14, 2023 •

edited

gradio-pr-bot commented Dec 14, 2023 •

edited

gradio-pr-bot commented Dec 14, 2023 •

edited