-
Notifications
You must be signed in to change notification settings - Fork 114
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] Update longpolling route for Odoo 16.0 #407
Conversation
_traefik2_labels.yml.jinja
Outdated
{{- | ||
router( | ||
domain_group=domain_group, | ||
key=key, | ||
suffix="longpolling", | ||
rule="%s && PathPrefix(`/longpolling/`)" % domains_rule(domain_group), | ||
rule="%s && PathPrefix(`%s`)" % (domains_rule(domain_group), longpolling_route), |
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.
According to my tests, the rule for <16 should be PathPrefix(`/longpolling/`)
as it was, but the rule for >= 16 should be Path(`/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.
Line 157 should take care of that, but apparently I mistyped /longpolling/
as /longpoling/
, should be fixed now.
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 mean that you don't want PathPrefix(`/websocket`)
, but Path(`/websocket`)
instead.
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 misread your first comment, sorry about that. Path
does indeed seem the correct rule, changed it.
@@ -20,8 +20,15 @@ services: | |||
ports: | |||
- "127.0.0.1:{{ macros.version_major(odoo_version) }}899:6899" | |||
- "127.0.0.1:{{ macros.version_major(odoo_version) }}069:8069" | |||
{%- if odoo_version >= 16 %} | |||
- "127.0.0.1:{{ macros.version_major(odoo_version) }}072:8072" |
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 don't think this is needed. Development is done in threaded mode, so only 1 port is needed.
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 agree that most users won't ever need it, but if for some reason someone tries to run devel.yaml
with --workers
> 1 (maybe they need to debug something in pre-fork mode), then it won't work.
Still, if you are sure, I will revert it.
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 that's the case, then don't restrict this to only v16+. Older versions also used that port, right?
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.
You're right, fixed it.
Can you please rebase to get a green CI? |
98a671c
to
21d457d
Compare
Was wondering why it was failing, didn't notice the new commits. Rebased now, thanks. |
As mentioned in Tecnativa/doodba#530, the old
/longpolling/
route was replaced by/websocket
in Odoo 16.0 (see odoo/odoo#75510). Currently, this results in the following error when a client tries to access the latter route (with the exception of threaded-mode odoo):This is caused by an undefined
socket
, which is only defined inenviron
when the request goes to the GeventServer's ProxyHandler or to the ThreadedServer, which uses a modified RequestHandler.For Doodba projects using Traefik, the solution is pretty straightforward: update the longpolling route so that WebSocket traffic passes through the
GeventServer
. Apparently, Traefik handles WebSocket requests the same as it would HTTP ones, as stated here, so no further changes should be required. I've personally tested the changes presented in this PR and had no issues.As for local environments running in pre-fork mode, I've applied the changes proposed by @yelizariev in Tecnativa/doodba#530, but making it work properly would still require the configuration of an external proxy such as Nginx.