Skip to content

Commit

Permalink
Revert "Optionally require websocket connection in react-client (#1379)…
Browse files Browse the repository at this point in the history
…" (#1547)

Reverts #1379

This is not working as intended. There are some race condition between events that we are subcribing to. So just revert and then we can implement a proper PR with tests.
  • Loading branch information
sandangel authored Nov 28, 2024
1 parent 8e739b3 commit a9f07b9
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 46 deletions.
7 changes: 3 additions & 4 deletions libs/react-client/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ This hook is responsible for managing the chat session's connection to the WebSo

#### Methods

- `connect`: Establishes a connection to the SocketIO server.
- `disconnect`: Disconnects from the SocketIO server.
- `connect`: Establishes a connection to the WebSocket server.
- `disconnect`: Disconnects from the WebSocket server.
- `setChatProfile`: Sets the chat profile state.

#### Example
Expand All @@ -60,8 +60,7 @@ const ChatComponent = () => {
userEnv: {
/* user environment variables */
},
accessToken: 'Bearer YOUR_ACCESS_TOKEN', // Optional Chainlit auth token
requireWebSocket: true // Optional, require WebSocket upgrade to be successful before user can interact with the chat bot. Will retry upgrade request every 500ms until successful. Please note if your server is behind a proxy, you will have to configure it to accept websocket upgrade request, otherwise users won't be able to interact with the app. You can check an example using nginx proxy here: https://nginx.org/en/docs/http/websocket.html. Default to false.
accessToken: 'Bearer YOUR_ACCESS_TOKEN' // Optional Chainlit auth token
});

return () => {
Expand Down
47 changes: 5 additions & 42 deletions libs/react-client/src/useChatSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,10 @@ const useChatSession = () => {
const _connect = useCallback(
({
userEnv,
accessToken,
requireWebSocket = false
accessToken
}: {
userEnv: Record<string, string>;
accessToken?: string;
requireWebSocket?: boolean;
}) => {
const { protocol, host, pathname } = new URL(client.httpEndpoint);
const uri = `${protocol}//${host}`;
Expand Down Expand Up @@ -121,49 +119,14 @@ const useChatSession = () => {
};
});

const onConnect = () => {
socket.on('connect', () => {
socket.emit('connection_successful');
setSession((s) => ({ ...s!, error: false }));
};
});

const onConnectError = () => {
socket.on('connect_error', (_) => {
setSession((s) => ({ ...s!, error: true }));
};

// https://socket.io/docs/v4/how-it-works/#upgrade-mechanism
// Require WebSocket when connecting to backend
if (requireWebSocket) {
// https://socket.io/docs/v4/client-socket-instance/#socketio
// 'connect' event is emitted when the underlying connection is established with polling transport
// 'upgrade' event is emitted when the underlying connection is upgraded to WebSocket and polling request is stopped.
const engine = socket.io.engine;
// https://github.com/socketio/socket.io/tree/main/packages/engine.io-client#events
engine.once('upgrade', () => {
// Set session on connect event, otherwise user can not interact with text input UI.
// Upgrade event is required to make sure user won't interact with the session before websocket upgrade success
socket.on('connect', onConnect);
});
// Socket.io will not retry upgrade request.
// Retry upgrade to websocket when error can only be done via reconnect.
// This will not be an issue for users if they are using persistent sticky session.
// In case they are using soft session affinity like Istio, then sometimes upgrade request will fail
engine.once('upgradeError', () => {
onConnectError();
setTimeout(() => {
socket.removeAllListeners();
socket.close();
_connect({
userEnv,
accessToken,
requireWebSocket
});
}, 500);
});
} else {
socket.on('connect', onConnect);
}

socket.on('connect_error', onConnectError);
});

socket.on('task_start', () => {
setLoading(true);
Expand Down

0 comments on commit a9f07b9

Please sign in to comment.