[VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper #27614

nicolas-grekas · Jun 15, 2018

Q	A
Branch?	4.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27622
License	MIT
Doc PR	-

Right now, the dump() function is broken on 4.1 as soon as one sets up a dump_destination for the dump server (as done by default by our Flex recipe). #27397 describes the issue and proposes a tentative fix. Yet, I think the issue is deeper and exists at the design level. Writting to the server should not happen in a DumperInterface, that's not its semantics. Instead, I propose a Connection object that will allow DumpDataCollector to have all the info it requires to do everything on its own.

My bad for not spotting this at the review stage.

nicolas-grekas

Now green (deps=high needs merge to master), ~~but untested on a real app~~.
Here are some comments to help @ogizanagi and others review.

nicolas-grekas · Jun 16, 2018

src/Symfony/Bundle/DebugBundle/DependencyInjection/DebugExtension.php

-
-        if (!isset($serverDumperHost)) {
-            $container->getDefinition('var_dumper.command.server_dump')->setClass(ServerDumpPlaceholderCommand::class);
-            if (!class_exists(ServerDumper::class)) {


this check is not needed since VarDumper is a mandatory requirement of DebugBundle

nicolas-grekas · Jun 16, 2018

src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php


        list('name' => $name, 'file' => $file, 'line' => $line, 'file_excerpt' => $fileExcerpt) = $this->sourceContextProvider->getContext();

-        if ($this->dumper) {
+        if ($this->dumper instanceof Connection) {
+            if (!$this->dumper->write($data)) {


this is the root of the issue: not being able to know if we wrote anything to the server to set $this->isCollected correctly

nicolas-grekas · Jun 16, 2018

src/Symfony/Component/VarDumper/Server/Connection.php

     * @param ContextProviderInterface[] $contextProviders Context providers indexed by context name
     */
-    public function __construct(string $host, DataDumperInterface $wrappedDumper = null, array $contextProviders = array())


I don't get why we need to have any wrapped dumper actually. This wrapped dumper currently cannot be configured, it's a hardcoded thing. Looks unneeded to me, is it?

Why do you think this isn't useful?
This is the mechanism that allows seamless usage of the server dumper with another dumper as fallback, meaning the behavior is the same as using the wrapped dumper directly when the server is not up.
I don't get what is "hardcoded" here.

nicolas-grekas · Jun 16, 2018

src/Symfony/Component/VarDumper/Server/Connection.php

-        $socket = stream_socket_client($this->host, $errno, $errstr, 1, STREAM_CLIENT_CONNECT | STREAM_CLIENT_PERSISTENT);
-
-        if ($socket) {
+        if ($socket = stream_socket_client($this->host, $errno, $errstr, 0, STREAM_CLIENT_CONNECT | STREAM_CLIENT_ASYNC_CONNECT | STREAM_CLIENT_PERSISTENT)) {


the equivalent monolog code connects in async mode, not sure why that's not the case here?

I had issues with async causing first dumps to not be sent to the server during the time the connection is up (IIRC). This is actually the same for the monolog code, at least when I tested it months ago.

nicolas-grekas · Jun 16, 2018

src/Symfony/Component/HttpKernel/EventListener/DumpListener.php

+            if ($connection) {
+                $connection->write($data);
+            }
+            $dumper->dump($data);


is it an issue to always write to the local console even when there is a connection?

I'd say yes. While running tests, it would output twice otherwise.

nicolas-grekas · Jun 17, 2018

src/Symfony/Component/VarDumper/Server/Connection.php

        $this->contextProviders = $contextProviders;
+        $this->socket = $this->createSocket();


Let's connect early. That's not an issue since this is async and it lets some time for the connection to be opened (might be useful: this is async)

Might be enough to workaround the async issue mentioned earlier, but not bullet proof I think.

nicolas-grekas · Jun 17, 2018

PR verified on a website-skeleton, works as expected now IMHO.

stof · Jun 18, 2018

src/Symfony/Bundle/DebugBundle/Resources/config/services.xml

-        <service id="var_dumper.server_dumper" class="Symfony\Component\VarDumper\Dumper\ServerDumper">
-            <argument>null</argument> <!-- server host -->
-            <argument type="service" id="var_dumper.cli_dumper" />
+        <service id="var_dumper.server_connection" class="Symfony\Component\VarDumper\Server\Connection">


should it be hidden ?

Right now, we hide only services with "random" names, not sure this PR should be the one changing this.

I don't think that's true. I'm quite sure I reviewed some PRs hiding some service names. But I'm still fine if the main rule is to hide only "random" names.

I didn't find any with a quick grep, let's let it as is.

stof · Jun 18, 2018

src/Symfony/Component/VarDumper/Server/Connection.php

 *
 * @author Maxime Steinhausser <maxime.steinhausser@gmail.com>
 */
-class ServerDumper implements DataDumperInterface
+class Connection


I suggest marking this class as @internal

With the suggested code, marking it @internal would mean someone using the component out of symfony full-stack cannot reliably wire this in their own application (we're saying them we can break this at any time).

stof · Jun 18, 2018

src/Symfony/Component/VarDumper/Server/ContextProvider/CliContextProvider.php

@@ -9,14 +9,16 @@
 * file that was distributed with this source code.
 */

-namespace Symfony\Component\VarDumper\Dumper\ContextProvider;
+namespace Symfony\Component\VarDumper\Server\ContextProvider;


should we actually rename these classes in a patch release ? they're not marked on @internal

That's my question here yes: are we OK to do with this BC break now, for something that really nobody (or very very few) will have used already? I'd like we answer by "yes"...

ogizanagi · Jun 18, 2018

Thanks for giving it a look :)

Writting to the server should not happen in a DumperInterface, that's not its semantics.

Well, I don't agree. DataDumperInterface is responsible for dumping the Data objects. Either locally in current output, or remotely letting a server delegate then to another dumper; that's not transgressing this responsibility.
You may keep the Connection object though, if you think it needs to be decoupled from the ServerDumper implementation, but the ServerDumper is legit to me. This is the one ensuring seamless usage of the server dumper when the server is not up. To me, it's a better design than requiring to set a handler with specific code for handling the connection (the changes you made in DumpListener). It's a solution well-integrated within the component, so better at a global level.
Please consider the DumpDataCollector issue we encounter here is the edge-case from the component POV. Not the norm. Someone using the VarDumper component alone should be able to wire the ServerDumper, IMHO with the current API.

To me, the culprit here really is the DumpDataCollector which has way too much responsibilities and hardcoded things.
We'd not have this issue if we had a DebugDumper (similar to the one proposed in #27397) that would have been set as the wrapped dumper of the ServerDumper, and the DumpDataCollector only dealing with its role of collecting the dumps (maybe with the classic TraceableDumper decorator around the wired dumper, i.e the server dumper when wired or debug dumper directly otherwise).

nicolas-grekas · Jun 18, 2018

DataDumperInterface is more for formatting than sending: HtmlDumper, CliDumper, JsonDumper, SerializerDumper, Base64SerializerDumper etc. these are all valid use cases for it. Since formatting needs an output medium, there is also an implicit concept of where the formatted Data should be written, by necessity. But this is a concept that is better separated from formatting. Maybe another name would have been better. The fact that the issue we're talking about exists at all is precisely the hint the interface has been misused to me. The fact also that by identifying this issue, this PR fixes it while removing lines of code is another hint.

nicolas-grekas · Jun 18, 2018

the culprit here really is the DumpDataCollector which has way too much responsibilities and hardcoded things

So, I tried again keepingServerDumper as done right now: this just doesn't fit, at least not without a maybe heavy refactoring. You may be right, but not in this reality :)

ogizanagi · Jun 18, 2018

at least not without a maybe heavy refactoring

Yes I also struggled with the DumpDataCollector (both when initially developing the feature and then with this issue). To me that's the hint it does too much.
I'd honestly prefer keeping the current API and work on #27397 to add the missing code from DumpDataColector even if it means duplicating code, until we can consider refactoring the DumpDataCollector in 4.2 according to #27614 (comment) last paragraph. So no BC break and the feature keeps being part of the classic VarDumper API.

You may be right, but not in this reality

Hope you're wrong ^^'

nicolas-grekas · Jun 20, 2018

I reverted the big BC break (there are still some minor on the edges, but this is required for the bugfix and OK for a .0 release IMHO.)

(failure will be fixed by a merge up to master)

ogizanagi

Tested and code looks good to me, thanks.

However, I encountered an issue that previously didn't exist: at least with php built-in webserver & bin/console server:run, as soon as the connection is lost (by shutting down the dump server previously up, executing a request and re-uping the server), the server won't get any input as stream_socket_sendto will throw a "Broken pipe" error.

Steps to reproduce:

bin/console server:run
bin/console server:dump
make a request calling dump(). At this stage, the dump is properly collected by the server.
Shutdown the server.
Make the same request again.
Re-up the server (bin/console server:dump). Do some more requests. Dumps will never reach the server until you restart the web server.

ogizanagi · Jun 20, 2018

src/Symfony/Component/VarDumper/Tests/Fixtures/dump_server.php

@@ -29,6 +29,8 @@

 $server->start();

+echo "READY\n";


nicolas-grekas · Jun 21, 2018

@ogizanagi I don't reproduce this behavior :(
Can you try this patch?

                 stream_socket_shutdown($this->socket, STREAM_SHUT_RDWR);
+                fclose($this->socket);
+                $this->socket = false;

ogizanagi · Jun 21, 2018

It works 👍

ogizanagi · Jun 21, 2018

It works, but only on second request after re-uping the server. Note that commenting the createSocket in the constructor is fixing it (just an hint of course).

nicolas-grekas · Jun 22, 2018

I removed the async flag + constructor connection, does it fix it?

ogizanagi · Jun 23, 2018

No, it doesn't work at all now, unless you restore a timeout in stream_socket_client too ("stream_socket_client(): unable to connect to tcp://127.0.0.1:9912 (Operation timed out)").

But honestly, if you want to give a try to async with previous version + patch above, I won't personally mind because I didn't reproduced on a different machine but with very similar env. We may reconsider if we have other reports. Or create another PR for async.

ogizanagi · Jun 23, 2018

src/Symfony/Component/VarDumper/Server/Connection.php

+namespace Symfony\Component\VarDumper\Server;
+
+use Symfony\Component\VarDumper\Cloner\Data;
+use Symfony\Component\VarDumper\Server\ContextProvider\ContextProviderInterface;


-use Symfony\Component\VarDumper\Server\ContextProvider\ContextProviderInterface; +use Symfony\Component\VarDumper\Dumper\ContextProvider\ContextProviderInterface;

nicolas-grekas · Jun 23, 2018

Pushed again, with your patch and the connection in the constructor removed.

ogizanagi

With typo fixed (and tests ok)

ogizanagi · Jun 23, 2018

src/Symfony/Component/VarDumper/Server/Connection.php

+
+    private function createSocket()
+    {
+        if ($socket = stream_socket_client($this->host, $errno, $errstr, 1, STREAM_CLIENT_CONNECT | STREAM_CLIENT_ASYNC | STREAM_CLIENT_PERSISTENT)) {


- STREAM_CLIENT_ASYNC + STREAM_CLIENT_ASYNC_CONNECT

…ServerDumper

… of Dumper/ServerDumper (nicolas-grekas) This PR was merged into the 4.1 branch. Discussion ---------- [VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27622 | License | MIT | Doc PR | - Right now, the `dump()` function is broken on 4.1 as soon as one sets up a `dump_destination` for the dump server (as done by default by our Flex recipe). #27397 describes the issue and proposes a tentative fix. Yet, I think the issue is deeper and exists at the design level. Writting to the server should not happen in a `DumperInterface`, that's not its semantics. Instead, I propose a `Connection` object that will allow `DumpDataCollector` to have all the info it requires to do everything on its own. My bad for not spotting this at the review stage. Commits ------- 1435d67 [VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper

nicolas-grekas added this to the 4.1 milestone Jun 15, 2018

carsonbot added Status: Needs Review VarDumper Bug BC Break labels Jun 15, 2018

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch 6 times, most recently from 67478cb to 18514e5 Compare June 16, 2018 09:16

nicolas-grekas commented Jun 16, 2018

View reviewed changes

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch 2 times, most recently from e7ffb40 to 575e072 Compare June 16, 2018 13:52

nicolas-grekas commented Jun 16, 2018

View reviewed changes

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch 2 times, most recently from f544f0c to 8b49b00 Compare June 17, 2018 07:24

nicolas-grekas commented Jun 17, 2018

View reviewed changes

This was referenced Jun 17, 2018

VarDumper in a controller => blank page #27622

Closed

VarDumper dump output not showing in debug toolbar #27621

Closed

stof reviewed Jun 18, 2018

View reviewed changes

ogizanagi mentioned this pull request Jun 19, 2018

[DebugBundle] Fix dump/die doesn't work with server dumper #27397

Closed

nicolas-grekas changed the title ~~[VarDumper] Replace Dumper/ServerDumper by Server/Connection~~ [VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper Jun 20, 2018

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch from 8b49b00 to 220cb20 Compare June 20, 2018 11:49

nicolas-grekas removed the BC Break label Jun 20, 2018

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch from 220cb20 to cc05eda Compare June 20, 2018 12:02

ogizanagi reviewed Jun 20, 2018

View reviewed changes

src/Symfony/Component/VarDumper/Tests/Fixtures/dump_server.php

@@ -29,6 +29,8 @@

$server->start();

echo "READY\n";

Copy link

Contributor

ogizanagi Jun 20, 2018

Choose a reason for hiding this comment

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

👍

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch from cc05eda to 0d22f42 Compare June 22, 2018 19:36

ogizanagi reviewed Jun 23, 2018

View reviewed changes

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch from 0d22f42 to ee6a631 Compare June 23, 2018 08:17

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 23, 2018

ogizanagi approved these changes Jun 23, 2018

View reviewed changes

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch 2 times, most recently from fc10fd7 to 65dcc8a Compare June 23, 2018 12:13

[VarDumper] Fix dumping by splitting Server/Connection out of Dumper/…

1435d67

…ServerDumper

nicolas-grekas force-pushed the fix/runtime_server_dumper_wrapped_dumper branch from 65dcc8a to 1435d67 Compare June 23, 2018 12:24

nicolas-grekas merged commit 1435d67 into symfony:4.1 Jun 24, 2018

nicolas-grekas deleted the fix/runtime_server_dumper_wrapped_dumper branch June 25, 2018 12:45

fabpot mentioned this pull request Jun 25, 2018

Release v4.1.1 #27706

Merged

ogizanagi mentioned this pull request Apr 22, 2022

[VarDumper] Server socket problem on OS X #46145

Closed

		$this->contextProviders = $contextProviders;
		$this->socket = $this->createSocket();

Search code, repositories, users, issues, pull requests...

Uh oh!

[VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper #27614

[VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper #27614

Uh oh!

Conversation

nicolas-grekas commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 18, 2018

Uh oh!

nicolas-grekas commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 21, 2018

Uh oh!

ogizanagi commented Jun 21, 2018

Uh oh!

ogizanagi commented Jun 21, 2018

Uh oh!

nicolas-grekas commented Jun 22, 2018

nicolas-grekas commented Jun 15, 2018 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

ogizanagi Jun 18, 2018 •

edited

Loading

ogizanagi commented Jun 18, 2018 •

edited

Loading

nicolas-grekas commented Jun 18, 2018 •

edited

Loading

ogizanagi commented Jun 18, 2018 •

edited

Loading

nicolas-grekas commented Jun 20, 2018 •

edited

Loading

ogizanagi left a comment •

edited

Loading

ogizanagi left a comment •

edited

Loading