Skip to content

[VarDumper] [bugfix] Make possible to create sync tcp-connection #47419

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[VarDumper] [bugfix] Make possible to create sync tcp-connection #47419

wants to merge 1 commit into from

Conversation

nikserg
Copy link

@nikserg nikserg commented Aug 29, 2022 •
edited by nicolas-grekas
Loading

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

On some systems creating async TCP connection results in "failed to open stream: Resource temporarily unavailable" exception. To solve this, added VAR_DUMPER_ASYNC variable, which is true by default, so won't affect default behavior.

If one faces "failed to open stream: Resource temporarily unavailable" error, he should add VAR_DUMPER_ASYNC=0 to .env file, and problem would be solved.

@carsonbot carsonbot added this to the 6.1 milestone Aug 29, 2022
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@xabbuh
Copy link
Member

xabbuh commented Sep 2, 2022

Is this change enough? Asking because the CI job on AppVeyor is still failing.

@nikserg
Copy link
Author

nikserg commented Sep 2, 2022

Is this change enough? Asking because the CI job on AppVeyor is still failing.

It says Uncaught InvalidArgumentException: Could not get class storage for static in phar:///home/runner/work/symfony/symfony/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Provider/ClassLikeStorageProvider.php:40

Can't see how it's connected to my changes.

@xabbuh
Copy link
Member

xabbuh commented Sep 2, 2022

see https://ci.appveyor.com/project/fabpot/symfony/builds/44608003#L1245

@nicolas-grekas nicolas-grekas modified the milestones: 6.1, 5.4 Dec 13, 2022
@nicolas-grekas
Copy link
Member

@xabbuh the failure is not related to this change (this is the server process failing to start for whatever reasons.)

@nikserg what about just always going in non-async mode? I don't like adding an option for something that is supposed to work out of the box. As far as I tested correctly locally, this would provide the same DX.

/cc @ogizanagi WDYT?

Side note: this is for 5.4

@nicolas-grekas
Copy link
Member

Could someone test on Windows please?

@ogizanagi
Copy link
Contributor

@nikserg what about just always going in non-async mode? I don't like adding an option for something that is supposed to work out of the box. As far as I tested correctly locally, this would provide the same DX.

I fully agree 👍🏻

@ogizanagi
Copy link
Contributor

@nikserg : Let's always go in non-async mode?

@nicolas-grekas
Copy link
Member

Closing as this stalled, please resubmit if anyone is up to do the last steps.

@ogizanagi ogizanagi mentioned this pull request Apr 18, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Apr 18, 2023
@nicolas-grekas
bug #50047 [VarDumper] Make the server TCP connection sync (ogizanagi)
6aacbd9 
This PR was merged into the 5.4 branch.

Discussion
----------

[VarDumper] Make the server TCP connection sync

| Q             | A
| ------------- | ---
| Branch?       | 5.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #46145 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT
| Doc PR        | N/A

Alternative to #47419, since the async mode is not consistently working and I agree with Nicolas [about not adding a new option](#47419 (comment)) to control this. Let's always go non-async.

Commits
-------

7580ada [VarDumper] Make the server TCP connection sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review VarDumper
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@nikserg @carsonbot @xabbuh @nicolas-grekas @ogizanagi