Skip to content

Commit

Permalink
Reduce number of references by discarding internal previous Exception
Browse files Browse the repository at this point in the history
  • Loading branch information
@clue
clue committed Sep 27, 2018
1 parent 8d396d6 commit 3abb49d
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 10 deletions.
3 changes: 1 addition & 2 deletions src/SecureConnector.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ public function connect($uri)

throw new \RuntimeException(
'Connection to ' . $uri . ' failed during TLS handshake: ' . $error->getMessage(),
0,
$error
$error->getCode()
);
});
});
Expand Down
3 changes: 1 addition & 2 deletions src/SecureServer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,7 @@ function ($conn) use ($that) {
function ($error) use ($that, $connection) {
$error = new \RuntimeException(
'Connection from ' . $connection->getRemoteAddress() . ' failed during TLS handshake: ' . $error->getMessage(),
$error->getCode(),
$error
$error->getCode()
);

$that->emit('error', array($error));
Expand Down
2 changes: 2 additions & 0 deletions tests/FunctionalSecureServerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ public function testEmitsErrorIfConnectionIsClosedBeforeHandshake()
$this->assertStringStartsWith('Connection from tcp://', $error->getMessage());
$this->assertStringEndsWith('failed during TLS handshake: Connection lost during TLS handshake', $error->getMessage());
$this->assertEquals(defined('SOCKET_ECONNRESET') ? SOCKET_ECONNRESET : 0, $error->getCode());
$this->assertNull($error->getPrevious());
}

public function testEmitsErrorIfConnectionIsClosedWithIncompleteHandshake()
Expand Down Expand Up @@ -452,6 +453,7 @@ public function testEmitsErrorIfConnectionIsClosedWithIncompleteHandshake()
$this->assertStringStartsWith('Connection from tcp://', $error->getMessage());
$this->assertStringEndsWith('failed during TLS handshake: Connection lost during TLS handshake', $error->getMessage());
$this->assertEquals(defined('SOCKET_ECONNRESET') ? SOCKET_ECONNRESET : 0, $error->getCode());
$this->assertNull($error->getPrevious());
}

public function testEmitsNothingIfConnectionIsIdle()
Expand Down
14 changes: 8 additions & 6 deletions tests/SecureConnectorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,13 @@ public function testStreamEncryptionWillBeEnabledAfterConnecting()
$promise = $this->connector->connect('example.com:80');
}

/**
* @expectedException RuntimeException
* @expectedExceptionMessage Connection to example.com:80 failed during TLS handshake: TLS error
*/
public function testConnectionWillBeRejectedIfStreamEncryptionFailsAndClosesConnection()
{
$connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
$connection->expects($this->once())->method('close');

$encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
$encryption->expects($this->once())->method('enable')->willReturn(Promise\reject(new \RuntimeException('TLS error')));
$encryption->expects($this->once())->method('enable')->willReturn(Promise\reject(new \RuntimeException('TLS error', 123)));

$ref = new \ReflectionProperty($this->connector, 'streamEncryption');
$ref->setAccessible(true);
Expand All @@ -128,7 +124,13 @@ public function testConnectionWillBeRejectedIfStreamEncryptionFailsAndClosesConn

$promise = $this->connector->connect('example.com:80');

$this->throwRejection($promise);
try {
$this->throwRejection($promise);
} catch (\RuntimeException $e) {
$this->assertEquals('Connection to example.com:80 failed during TLS handshake: TLS error', $e->getMessage());
$this->assertEquals(123, $e->getCode());
$this->assertNull($e->getPrevious());
}
}

/**
Expand Down

6 comments on commit 3abb49d

@WyriHaximus
Copy link
Member

Choose a reason for hiding this comment

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

Not really a fan of discarding the previous exception(s). Is clone an alternative to consider?

@clue
Copy link
Member Author

@clue clue commented on 3abb49d Sep 27, 2018

Choose a reason for hiding this comment

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

In general I agree that previous (or "nested") exceptions make perfect sense to get a better understanding of what caused a certain exceptional situation. In this particular instance, it's my understanding that it does not add any value because all the info is already included in the RuntimeException returned from the SecureConnector.

In the old version, the exception message would not include the connection target at all. The new exception message now looks like Connection to example.com:80 failed during TLS handshake: TLS error where the nested exception would only repeat the same error message TLS error. This is now also consistent across this component.

What do you think about this?

@jsor
Copy link
Member

@jsor jsor commented on 3abb49d Sep 27, 2018

Choose a reason for hiding this comment

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

It's not only the message, but also the original stack trace which gets discarded.

@clue
Copy link
Member Author

@clue clue commented on 3abb49d Sep 27, 2018

Choose a reason for hiding this comment

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

@jsor Which is in fact also intentional. It does not contain any meaningful data that is not present in the exception trace that is returned from the SecureConnector. Note that the StreamEncryption class is @internal only and nothing any of our consumers should have to care about.

@clue
Copy link
Member Author

@clue clue commented on 3abb49d Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For the reference, the following code is expected to reject with an exception:

$promise = $connector->connect('tls://google.com:80');

$promise->then(null, function ($e) {
    echo $e;
});

Take a look at the default output for this command, I honestly don't think the nested exception adds any value here (arguably it's even less clear). For the reference, here's also the full trace and the var_dump() output: https://gist.github.com/clue/a478dd49b4ace7ffa9828585548b0888/revisions

@clue
Copy link
Member Author

@clue clue commented on 3abb49d Sep 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

After discussing this with @WyriHaximus in private it seems I should have included some example error messages to begin with :-) Here's what the original exception message looked like before applying this PR:

Unable to complete TLS handshake: SSL operation failed with code 1. OpenSSL Error messages: error:1408F10B:SSL routines:ssl3_get_record:wrong version number

In particular, note that while this includes a lot of low level details and is rather cryptic it does not include the basic information that this means that a connection attempt just failed and which connection was affected. This PR updates the exception to always include some higher level message and then always appends the exact underlying error message like this:

Connection to google.com:80 failed during TLS handshake: Unable to complete TLS handshake: SSL operation failed with code 1. OpenSSL Error messages: error:1408F10B:SSL routines:ssl3_get_record:wrong version number

This means that this PR does in fact contain all the information that was already present previously. In addition, it makes it much clearer what the cause of this exception actually is. This applies to both the client side as in the previous example and also to the server side which now includes the remote IP so the error event can actually be used to diagnose issues:

Connection from tcp://127.0.0.1:39528 failed during TLS handshake: Connection lost during TLS handshake

Please sign in to comment.