-
Notifications
You must be signed in to change notification settings - Fork 7.6k
fix(polls): Robust room resolution and logging for polls in breakout rooms #16695
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
base: master
Are you sure you want to change the base?
Conversation
|
@damencho could you please take a look at this solution. |
|
Hi, thanks for your contribution! |
| module:log('error', '[Polls] No room found for session context: jitsi_web_query_room=%s, jitsi_web_query_prefix=%s, stanza.from=%s, stanza.to=%s', | ||
| tostring(session.jitsi_web_query_room), tostring(session.jitsi_web_query_prefix), tostring(stanza.attr.from), tostring(stanza.attr.to)); | ||
| -- Fallback: try to infer room from occupant's current room if in a breakout room context | ||
| if session.jitsi_web_query_room == nil or session.jitsi_web_query_prefix == nil then |
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.
why would these be nil, they will always be set. Do you see somewhere these to be cleared?
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.
These variables (session.jitsi_web_query_room and session.jitsi_web_query_prefix) should indeed always be set during normal operation. However, this fallback is a defensive measure for unexpected edge cases, such as:
- Integration with custom or third-party Prosody modules that might not set these variables correctly.
- Rare race conditions or errors during room transitions (especially with breakout rooms).
- Potential future changes or bugs that could cause the session context to be incomplete.
I have not found any standard code that explicitly clears these variables, but the fallback ensures that, if for any reason they are missing, the system can still attempt to resolve the room and log the issue for easier debugging. This is a safeguard to make the system more robust in production.
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 like this approach, there are plenty of other places that these two are used, should we add it everywhere.
If someone has a bug and has cleared them or not set them that needs to be fixed.
And this will not solve the polls problem.
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.
yeah if I think about it, my approach would be only a defensive approach instead of solving the issue. Let me find where the issue is originating from.
…bust poll delivery
|
In fact, we could add this change so that when the fallback successfully resolves the room, it also updates the session context ( |
…t rooms - Set jitsi_web_query_room and jitsi_web_query_prefix for all occupants on every room join - Remove defensive fallback from polls component - Enforce LF line endings for consistency
|
@damencho removed the fallback option but added a proper root cause fix by updating session context on all room joins. This ensures jitsi_web_query_room and jitsi_web_query_prefix are set for every occupant in breakout rooms, resolving the polls issue without workarounds. |
|
I see, but I don't like this approach. I will think about it |
|
Maybe the way to go is adding roomjid and doing validation on it, but I need some time to look how does the signalling looks like |
@damencho |
This PR improves the reliability of poll delivery in breakout rooms by enhancing room resolution logic in [mod_polls_component.lua].
Fixes issue #16693