Skip to content
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

Broadcasters with names that contain both { and } are never cleaned up #2487

Open
alexkau opened this issue Feb 10, 2023 · 1 comment
Open
Assignees

Comments

@alexkau
Copy link
Contributor

alexkau commented Feb 10, 2023

Describe the bug
Broadcasters with names that contain both { and } are never cleaned up.

Atmosphere Info
2.7.4 (bug introduced in 2.6.0)

Repro Steps
Configure a lifecycle strategy, such as IDLE_EMPTY_DESTROY, which should cause the broadcaster to automatically close at some point.
Create a broadcaster that uses both { and } in its name, and one that does not.
Disconnect from the broadcasters and wait until they should be destroyed.

Expected behavior
Both broadcasters are destroyed.

Actual results
The broadcaster with { and } in its name is never destroyed.

Additional context
This was introduced by the fix for #2407, and is coming from the first few lines in LifecycleHander:
b5d4c9a#diff-bdbb29403dc693807ab47591ccfa632a3790cc0751cf72b0ece5082b3eb22e19

        if (broadcaster.getID().contains("{") && broadcaster.getID().contains("}")) {
            logger.trace("Ignoring wildcard {} with lifecycle policy: {}", broadcaster.getID(), lifeCyclePolicy.getLifeCyclePolicy().name());
            return this;
        }

I'm not able to find any mention of this change in the wiki, and cannot find any release notes for 2.6 at all, so this undocumented change has caused my application to build up an excessive number of broadcasters for about the last year since we upgraded to 2.7.

We're implementing a workaround to avoid putting { and } in our broadcaster names, but I wanted to file this anyways since this limitation -- if it is intentional -- should really be documented.

@jfarcand
Copy link
Member

@alexkau Nice catch. Clearly a regression I've introduced. Will take a look once I have cycle, or improve the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants