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

[HttpKernel][WebProfilerBundle] Fix search feature #50913

Merged
merged 1 commit into from Jul 13, 2023

Conversation

Pelagoss
Copy link

@Pelagoss Pelagoss commented Jul 7, 2023

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

This fix research feature in the profiler.

Sometimes lines in index.csv are broken with something like this :

,,200

the try and catch allow the profiler to skip these lines

@GromNaN
Copy link
Member

GromNaN commented Jul 7, 2023

Sometimes lines in index.csv are broken

It looks like you're trying to hide a problem in FileProfilerStorage. Do you know why there is invalid rows in the file?

@Pelagoss
Copy link
Author

Pelagoss commented Jul 8, 2023

I think it only occurs when the file is very big. Or maybe when multiple client requests the backend at same time

@MatTheCat
Copy link
Contributor

The root issue seems to be the same as #50816.

@Pelagoss
Copy link
Author

I will try with 6.3 and let you know

@Pelagoss
Copy link
Author

Pelagoss commented Jul 10, 2023

image
image
In this exemple (6.2.12), I dump everytime a line is writed in the .csv and look the line with only a "0", it corresponds with neither dump

@nicolas-grekas
Copy link
Member

Thank you @Pelagoss.

@nicolas-grekas nicolas-grekas merged commit e8dc1c2 into symfony:5.4 Jul 13, 2023
6 of 11 checks passed
nicolas-grekas added a commit that referenced this pull request Jul 13, 2023
…age (radar3301)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpKernel] Skip corrupted CSV data in FileProfilerStorage

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Partially fixes #50942, improves/fixes on PR #50913, partially fixes #50816
| License       | MIT

Technically, part of this fix could go all the way back to 2.1, but since we're not sure why/how exactly the index.csv file is being corrupted in the first place, this PR should just serve as a stop-gap measure until the root cause can be identified.

A corrupted index.csv file is especially noticeable since 6.3, when the "Remove Expired Profiles" feature was added by #47352 .

RE: improves/fixes on PR #50913:
Warnings are (usually?) only escalated to errors in debug mode, so having a try/catch is a bit more overhead than I think is actually needed, and I believe just checking that the line read from the csv is not corrupted is the better/faster option. Granted, the profiler should really only be active in debug/non-production modes anyway, but "should" and reality usually don't align...

`@alamirault` `@MatTheCat` `@benjaminfunk` `@Pelagoss` `@derrabus`

Commits
-------

b4e942d Update FileProfilerStorage.php
This was referenced Jul 29, 2023
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.

None yet

6 participants