-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor websocket #4879
Refactor websocket #4879
Conversation
// Strict mode mounts and unmounts each component twice, so we have to wait in the destructor | ||
// before actually closing the socket and cancel the operation if the component gets remounted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually have a useEffectOnce
!
OpenHands/frontend/src/utils/use-effect-once.ts
Lines 7 to 18 in bf8ccc8
export const useEffectOnce = (callback: () => void) => { | |
const isUsedRef = React.useRef(false); | |
React.useEffect(() => { | |
if (isUsedRef.current) { | |
return; | |
} | |
isUsedRef.current = true; | |
callback(); | |
}, [isUsedRef.current]); | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that we may also be able to avoid persisting timeout
(or having a timeout altogether) across re-renders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseEffectOnce
will execute the effect once the first time the component mounts, but then not again after this, which means if the tokens or enabled flag changes then the effect will not re-execute.
I thought about adding a custom effect useEffectIfActuallyChanged
, but decided against it because this is what useEffect
is actually meant to do, and it only does not to test whether components actually mount and unmount properly. Our websocket is a rare corner case where we don't want the mount / unmount behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to the one below the comment which is already executed only once when the component mounts, as implied by the empty dependency array, not the one above 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstood? I was talking about this one:
React.useEffect(() => {
const timeout = closeRef.current;
if (timeout != null) {
clearTimeout(timeout);
}
return () => {
closeRef.current = setTimeout(() => {
wsRef.current?.close();
}, 100);
};
}, []);
In this case, if I useEffectOnce
...
useEffectOnce
does not return a destructor - the destructor is what I am primarily interested in here - it is where I close the websocket if it is still open- Even if it did return a destructor, when should it be invoked? It would need timeout logic like I have - otherwise the destructor would always be invoked the first time the component is unmounted, closing the websocket.
<p className="text-xs text-danger"> | ||
Changing settings during an active session will end the session | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually may be annoying. With these changes we're either always going to show this error or never since the parent does not have access to the context. 🤔
@@ -20,6 +20,7 @@ export const VERIFIED_ANTHROPIC_MODELS = [ | |||
"claude-2", | |||
"claude-2.1", | |||
"claude-3-5-sonnet-20240620", | |||
"claude-3-5-sonnet-20241022", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is to prevent the failing test, I've already fixed it in #4887
export function handleAssistantMessage(message: Record<string, unknown>) { | ||
if (message.action) { | ||
handleActionMessage(message as unknown as ActionMessage); | ||
} else if (message.observation) { | ||
handleObservationMessage(message as unknown as ObservationMessage); | ||
} else if (message.status_update) { | ||
handleStatusMessage(message as unknown as StatusMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW we have some improved (but still unused) types in frontend/src/types/core. No need to change them yet, just making you aware of it if you aren't already
Co-authored-by: sp.wack <[email protected]>
Co-authored-by: sp.wack <[email protected]>
Refactor client side web socket control for better maintainability.
So, when implementing the reconnect, I kept hitting walls. Eventually I figured some of the problem was the way we use sockets on the client side - it feels that we are trying to use it like JQuery. This refactor:
This will make reconnect logic far easier to implement, as it can mostly be isolated to
WsClientProvider
.To run this PR locally, use the following command: