Skip to content

[Console] Use a partial buffer in SymfonyStyle #39160

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

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

jderusse
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39156
License MIT
Doc PR -

Symfony style needs to buffer output in order to get the last 2 chars in order to prepend Block.

By using a BufferedOutput symfony bufferize everything while it not needed.

This PR adds a new TrimmedBufferOutput that keep only the N last chars.

@jderusse jderusse requested a review from chalasr as a code owner November 24, 2020 12:49
@jderusse jderusse changed the title Use a partial buffer in SymfonyStyle [Console] Use a partial buffer in SymfonyStyle Nov 24, 2020
@jderusse jderusse added this to the 4.4 milestone Nov 24, 2020
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug exists on lower branches, isn't it?
Can we mark the new class as internal to make this target 4.4?

@jderusse jderusse changed the base branch from 5.x to 4.4 November 24, 2020 13:12
@jderusse
Copy link
Member Author

The bug exists on lower branches, isn't it?
Can we mark the new class as internal to make this target 4.4?

Thanks, I picked the wrong base branch when creating the PR

@jderusse jderusse force-pushed the buffer-overflow branch 2 times, most recently from fd29aef to 3d4249c Compare November 24, 2020 14:11
@javiereguiluz
Copy link
Member

I'm not sure we need to fix this "issue". In the original issue report, the reproducer in #39156 is a code that writes a string 26 millions of times. How is that realistic?

@stof
Copy link
Member

stof commented Nov 24, 2020

@javiereguiluz the original laravel issue was abotu a long-running command outputting lots of different things. The minimal reproducer is not meant to be a realistic case, but a minimal case.

@javiereguiluz
Copy link
Member

The original Laravel issue report says that this problem occurs "After running it for a long time". How long is "long" considered here? Hours? Days? Weeks? I wouldn't try to fix this because for now it looks like a "non issue". (In PHP you must restart long-running processes from time to time to avoid these "memory leaks" or other issues, right?)

@jderusse
Copy link
Member Author

The script leaks. The fix is not that complexe...
I don't understand why we shouldn't fix it...

In PHP you must restart long-running processes from time to time to avoid these "memory leaks" or other issues, right?)

NOooo!!!! Why people believe that having a memory leak is the normal behavior?

I hope I won't offend you @javiereguiluz , but I'm really tired of people saying PHP sucks and have a bad memory management.
Running a PHP script for days is perfectly fine!

@z5864703
Copy link

The original Laravel issue report says that this problem occurs "After running it for a long time". How long is "long" considered here? Hours? Days? Weeks? I wouldn't try to fix this because for now it looks like a "non issue". (In PHP you must restart long-running processes from time to time to avoid these "memory leaks" or other issues, right?)

If this is not the problem, our script can run forever without memory leaks.

This has nothing to do with the length of time the problem occurred. If the business needs it, the trigger point can be reached in a day. If this function is not used, it will never be triggered. What is the significance of this function? Knowing that there is a problem that can be solved, but through a method that is not a solution?

fabpot added a commit that referenced this pull request Nov 25, 2020
This PR was merged into the 4.4 branch.

Discussion
----------

[Console] Fix console closing tag

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

When using SymfonyStyle, in some cases, closing a tag, is called twice.
In the following code `<question>do you want <comment>something</>?</>` the first `</>` close both the `comment` and the `question` tags.

![Screenshot from 2020-11-25 01-21-06](https://user-images.githubusercontent.com/578547/100166475-191d9d80-2ebd-11eb-991a-6541210c479b.png)

The reason is, part of the content is sent in 2 Outputs (see #39160 for another issue), and both outputs share the same `$styleStack`.
This PR updates the `OutputFormatter::__clone` method to prevent sharing the same state.

Commits
-------

2834c27 Fix console closing tag
@chalasr
Copy link
Member

chalasr commented Nov 25, 2020

Thank you @jderusse.

@chalasr chalasr merged commit fd910eb into symfony:4.4 Nov 25, 2020
@derrabus
Copy link
Member

@jderusse: Can you please have a look at b047064? When merging your changes to 5.1, I had to add an extra line to the test to make it pass again.

@jderusse
Copy link
Member Author

@jderusse: Can you please have a look at b047064? When merging your changes to 5.1, I had to add an extra line to the test to make it pass again.

Thanks for pinging. Yes, you fix is OK.
I'm not sure why it broke, but the idea of the tests remains the same:

  1. warm the buffer
  2. memorize the size of the memory
  3. run the script several times
  4. assert the size didn't changed

@derrabus
Copy link
Member

Yes, the test still proves that the leak is stopped. 👍🏻

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

Successfully merging this pull request may close these issues.

Component/Console/Output/BufferedOutput buffer memory overflow
7 participants