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

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

Merged
merged 1 commit into from Jun 24, 2018

Conversation

Projects
None yet
4 participants
@nicolas-grekas
Member

nicolas-grekas commented 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.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 16, 2018

Member

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

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 comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 16, 2018

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 16, 2018

Member

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?

This comment has been minimized.

@ogizanagi

ogizanagi Jun 18, 2018

Member

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.

$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)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 16, 2018

Member

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

This comment has been minimized.

@ogizanagi

ogizanagi Jun 18, 2018

Member

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.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 16, 2018

Member

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

This comment has been minimized.

@ogizanagi

ogizanagi Jun 18, 2018

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 17, 2018

Member

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)

This comment has been minimized.

@ogizanagi

ogizanagi Jun 18, 2018

Member

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

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 17, 2018

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

<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">

This comment has been minimized.

@stof

stof Jun 18, 2018

Member

should it be hidden ?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 18, 2018

Member

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

This comment has been minimized.

@stof

stof Jun 20, 2018

Member

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 20, 2018

Member

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

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

This comment has been minimized.

@stof

stof Jun 18, 2018

Member

I suggest marking this class as @internal

This comment has been minimized.

@ogizanagi

ogizanagi Jun 18, 2018

Member

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).

@@ -9,14 +9,16 @@
* file that was distributed with this source code.
*/
namespace Symfony\Component\VarDumper\Dumper\ContextProvider;
namespace Symfony\Component\VarDumper\Server\ContextProvider;

This comment has been minimized.

@stof

stof Jun 18, 2018

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 18, 2018

Member

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

This comment has been minimized.

Member

ogizanagi commented 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

This comment has been minimized.

Member

nicolas-grekas commented 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

This comment has been minimized.

Member

nicolas-grekas commented 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

This comment has been minimized.

Member

ogizanagi commented 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 nicolas-grekas changed the title from [VarDumper] Replace Dumper/ServerDumper by Server/Connection to [VarDumper] Fix dumping by splitting Server/Connection out of Dumper/ServerDumper Jun 20, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented 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:

  1. bin/console server:run
  2. bin/console server:dump
  3. make a request calling dump(). At this stage, the dump is properly collected by the server.
  4. Shutdown the server.
  5. Make the same request again.
  6. Re-up the server (bin/console server:dump). Do some more requests. Dumps will never reach the server until you restart the web server.
@@ -29,6 +29,8 @@
$server->start();
echo "READY\n";

This comment has been minimized.

@ogizanagi
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented 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

This comment has been minimized.

Member

ogizanagi commented Jun 21, 2018

It works 👍

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented 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

This comment has been minimized.

Member

nicolas-grekas commented Jun 22, 2018

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

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented 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.

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

This comment has been minimized.

@ogizanagi

ogizanagi Jun 23, 2018

Member
-use Symfony\Component\VarDumper\Server\ContextProvider\ContextProviderInterface;
+use Symfony\Component\VarDumper\Dumper\ContextProvider\ContextProviderInterface;
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 23, 2018

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

@ogizanagi

ogizanagi approved these changes Jun 23, 2018 edited

With typo fixed (and tests ok)

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

This comment has been minimized.

@ogizanagi

ogizanagi Jun 23, 2018

Member
- STREAM_CLIENT_ASYNC
+ STREAM_CLIENT_ASYNC_CONNECT

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

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 24, 2018

bug #27614 [VarDumper] Fix dumping by splitting Server/Connection out…
… 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 nicolas-grekas deleted the nicolas-grekas:fix/runtime_server_dumper_wrapped_dumper branch Jun 25, 2018

@fabpot fabpot referenced this pull request Jun 25, 2018

Merged

Release v4.1.1 #27706

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