-
Notifications
You must be signed in to change notification settings - Fork 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
fix: pass headers when connecting sockets #1575
Conversation
This is failing for any origin that is the not the server's origin (typically Chainlit embedded as a copilot on a website) because
|
The issue is fixed. For cross origin use cases, it is now mandatory to allow it in the project config (I had to update the copilot test for example) as the wildcard |
@@ -100,6 +100,8 @@ const useChatSession = () => { | |||
|
|||
const socket = io(uri, { | |||
path, | |||
withCredentials: true, | |||
transports: ['websocket'], |
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 change works for our use-case, but I believe adding the transports
option here means that Socket.IO will not fall back to long-polling if a WS connection can't be established. There are probably still some load balancers/browsers out there that can't handle web-sockets properly. It might be helpful for these options to become chainlit initialization options that the user can select?
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.
For instance the gcp load balancer only works with this. We can make it configurable I think.
} | ||
withCredentials: true, | ||
transports: ['websocket'], | ||
auth: { |
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.
Had to move out from the extra header approach since the websocket transport does not support that.
@@ -311,6 +311,8 @@ class CodeSettings: | |||
@dataclass() | |||
class ProjectSettings(DataClassJsonMixin): | |||
allow_origins: List[str] = Field(default_factory=lambda: ["*"]) | |||
# Socket.io client transports option | |||
transports: Optional[List[str]] = None |
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.
made the transports configurable @evantahler
Here is the doc PR Chainlit/docs#199 |
I gave this branch a try within our application, but every request I made is rejected:
We have a custom data layer, but I tried removing it with the same (failing) results. Any tips? |
Note: Poetry install line for
|
Convenient one-liner - not requiring authentication/write access to the repo: poetry add git+https://github.com/Chainlit/chainlit.git@willy/sticky-sessions#subdirectory=backend/ |
Could you share a screendump of the browser's console and network requests from the debugger? Particularly the websocket initialisation process. Details on the first non-successful request from the network debugger would also help. |
Incidentally, a prior PR for this issue was already open #1339. Should close it once we merge this. |
Thanks. That was quick! Could you send the browser's Console as well? I am about to test with a similar setup for our own application RN. |
@evantahler The auth error comes from here: chainlit/backend/chainlit/socket.py Line 117 in 8b2d4ba
Could you remove the try/except there, or change it into |
The browser console for this page is just tons of react errors about tooltips and mimetypes. Here's the output in a gist - https://gist.github.com/evantahler/171f6aa0a9f862c0122ec0728d8c430c |
🤣 |
GTG now, happy to look again tomorrow. Any discriminating factors (what auth are you using) and/or feedback from other users greatly appreciated. |
Looks like my
(I added some line breaks) |
Yes, that seems most likely. Can you share your Dockerfile? Normally, However, if you are running a chainlit install from the repo (as should be the case for source builds), AFAIK it serves from The obvious way is to add a |
Most likely using an old version of the UI. The auth change happens in the react-client (which is supposed to be rebuilt as well by |
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've just done repeated login/logout testing with 4 instances on GCP with OAuth. Used private browser and normal browser. No problems whatsoever. I think we should merge this and have users test it in a release candidate.!
@@ -13,7 +13,7 @@ session_timeout = 3600 | |||
cache = false | |||
|
|||
# Authorized origins | |||
allow_origins = ["*"] | |||
allow_origins = ["http://127.0.0.1:8000"] |
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 love finally seeing this! :)
We should document that on a server users need to configure the origin.
except StopIteration: | ||
return str(uuid.uuid5(uuid.NAMESPACE_DNS, ip)) | ||
|
||
|
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.
Happy to see this go! :)
@@ -149,7 +132,7 @@ def emit_call_fn(event: Literal["ask", "call_fn"], data, timeout): | |||
user=user, | |||
token=token, | |||
chat_profile=chat_profile, | |||
thread_id=environ.get("HTTP_X_CHAINLIT_THREAD_ID"), | |||
thread_id=auth.get("threadId"), |
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.
Seems much cleaner here.
Excellent! Can you comment here when that RC is ready to try out? |
@willydouhard @dokterbob Just installed chainlit from main branch and got the same message about UPD. I confirm downgrading to previous commit resolves the issue. UPD2. It looks like pip isn't propely downgrade packages, so if you install chainlit via git, you want to fully recreate environment in order to downgrade and fix an issue. |
I am not able to reproduce, did you rebuild the entire UI including the react client? The auth change is actually in the react client which is a dependency of the main frontend. |
The pre-release is available @evantahler @asvishnyakov.
Let me know if it works on your end. |
@willydouhard Thank you! This resolved the issue. Glad to be able use new version, it has amazing features! |
We are using a custom frontend ( |
I just released it, version
|
Fixing #1279 and closes #1359 as well. Tested it on GCP with session affinity + 2 instances with this docker file