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

Do not mutate EventDispatcher at runtime #127

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@Baldinof
Copy link
Contributor

Baldinof commented May 11, 2019

Hi,

I am playing with servers that allows a kernel to handle multiple requests (php-pm, roadrunner).

The current version of the CorsListener leads to memory leaks in this context (adding the same listeners on each request).

In this PR:

  • onKernelRequest stores flags in the request attributes instead of adding listener on the dispatcher
  • onKernelResponse is always registered and checks these flags to choose what needs to be done
@Baldinof

This comment has been minimized.

Copy link
Contributor Author

Baldinof commented May 16, 2019

Ping @dunglas (you seems to be the last committer)

@nicolas-grekas

This comment has been minimized.

Copy link

nicolas-grekas commented Nov 5, 2019

Since this is a BC break, this PR could be a good fit after #136, if v2 is confirmed.
@Baldinof would you be available to rebase it after #136 is merged?

@Baldinof

This comment has been minimized.

Copy link
Contributor Author

Baldinof commented Nov 5, 2019

Yes :)

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 6, 2019

Ready for rebase if you have time @Baldinof

@Baldinof

This comment has been minimized.

Copy link
Contributor Author

Baldinof commented Nov 6, 2019

Rebase done!

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 6, 2019

Thanks. I think this looks good but still happy to get a quick review @nicolas-grekas if you have time.

Copy link

nicolas-grekas left a comment

LGTM

@Seldaek Seldaek merged commit db42ecb into nelmio:master Nov 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Baldinof

This comment has been minimized.

Copy link
Contributor Author

Baldinof commented Nov 6, 2019

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.