Skip to content

Commit c640cdc

Browse files
boenrobotboenrobot
committed
Moved the timeout responsibility to the Response object, which will in turn only use it when it's ACTUALLY about to receive - not when it's pulling data out of a Registry or whatever;
Also added a timeout to Client::login() (Client::__construct() uses the connection timeout as a value for it), in order to address an accidentally discovered edge case - a TCP connection that's reported as being established, but never brings back any data.
1 parent 04dff7d commit c640cdc

File tree

4 files changed

+61
-39
lines changed

4 files changed

+61
-39
lines changed

‎src/PEAR2/Net/RouterOS/Client.php‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -123,12 +123,15 @@ public function __construct(
123123
$username . '/' . $password,
124124
$context
125125
);
126+
if (null == $timeout) {
127+
$timeout = ini_get('default_socket_timeout');
128+
}
126129
//Login the user if necessary
127130
if ((!$persist
128131
|| !($old = $this->com->getTransmitter()->lock(S::DIRECTION_ALL)))
129132
&& $this->com->getTransmitter()->isFresh()
130133
) {
131-
if (!static::login($this->com, $username, $password)) {
134+
if (!static::login($this->com, $username, $password, $timeout)) {
132135
$this->com->close();
133136
throw new DataFlowException(
134137
'Invalid username or password supplied.',
@@ -183,8 +186,12 @@ public function __invoke($arg = null)
183186
*
184187
* @return bool TRUE on success, FALSE on failure.
185188
*/
186-
public static function login(Communicator $com, $username, $password = '')
187-
{
189+
public static function login(
190+
Communicator $com,
191+
$username,
192+
$password = '',
193+
$timeout = null
194+
) {
188195
if (null !== ($remoteCharset = $com->getCharset($com::CHARSET_REMOTE))
189196
&& null !== ($localCharset = $com->getCharset($com::CHARSET_LOCAL))
190197
) {
@@ -198,11 +205,11 @@ public static function login(Communicator $com, $username, $password = '')
198205
try {
199206
if ($com->getTransmitter()->isPersistent()) {
200207
$old = $com->getTransmitter()->lock(S::DIRECTION_ALL);
201-
$result = self::_login($com, $username, $password);
208+
$result = self::_login($com, $username, $password, $timeout);
202209
$com->getTransmitter()->lock($old, true);
203210
return $result;
204211
}
205-
return self::_login($com, $username, $password);
212+
return self::_login($com, $username, $password, $timeout);
206213
} catch (\Exception $e) {
207214
if ($com->getTransmitter()->isPersistent() && null !== $old) {
208215
$com->getTransmitter()->lock($old, true);
@@ -233,11 +240,12 @@ public static function login(Communicator $com, $username, $password = '')
233240
private static function _login(
234241
Communicator $com,
235242
$username,
236-
$password
243+
$password = '',
244+
$timeout = null
237245
) {
238246
$request = new Request('/login');
239247
$request->send($com);
240-
$response = new Response($com);
248+
$response = new Response($com, false, $timeout);
241249
$request->setArgument('name', $username);
242250
$request->setArgument(
243251
'response',
@@ -247,7 +255,7 @@ private static function _login(
247255
)
248256
);
249257
$request->send($com);
250-
$response = new Response($com);
258+
$response = new Response($com, false, $timeout);
251259
return $response->getType() === Response::TYPE_FINAL
252260
&& null === $response->getArgument('ret');
253261
}
@@ -531,7 +539,7 @@ public function loop($timeout_s = null, $timeout_us = 0)
531539
}
532540
}
533541
} catch (SocketException $e) {
534-
if ($e->getCode() !== 11800) {
542+
if ($e->getCode() !== 50000) {
535543
// @codeCoverageIgnoreStart
536544
// It's impossible to reliably cause any other SocketException.
537545
// This line is only here in case the unthinkable happens:
@@ -738,19 +746,11 @@ protected function send(Request $request)
738746
*/
739747
protected function dispatchNextResponse($timeout_s = 0, $timeout_us = 0)
740748
{
741-
if (!$this->com->getTransmitter()->isDataAwaiting(
742-
$timeout_s,
743-
$timeout_us
744-
)
745-
) {
746-
throw new SocketException(
747-
'No responses within the time limit',
748-
11800
749-
);
750-
}
751749
$response = new Response(
752750
$this->com,
753751
$this->_streamingResponses,
752+
$timeout_s,
753+
$timeout_us,
754754
$this->registry
755755
);
756756
if ($response->getType() === Response::TYPE_FATAL) {

‎src/PEAR2/Net/RouterOS/Response.php‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,20 +84,22 @@ class Response extends Message
8484
public function __construct(
8585
Communicator $com,
8686
$asStream = false,
87+
$timeout_s = 0,
88+
$timeout_us = null,
8789
Registry $reg = null
8890
) {
8991
if (null === $reg) {
9092
if ($com->getTransmitter()->isPersistent()) {
9193
$old = $com->getTransmitter()
9294
->lock(T\Stream::DIRECTION_RECEIVE);
93-
$this->_receive($com, $asStream);
95+
$this->_receive($com, $asStream, $timeout_s, $timeout_us);
9496
$com->getTransmitter()->lock($old, true);
9597
} else {
96-
$this->_receive($com, $asStream);
98+
$this->_receive($com, $asStream, $timeout_s, $timeout_us);
9799
}
98100
} else {
99101
while (null === ($response = $reg->getNextResponse())) {
100-
$newResponse = new self($com, true);
102+
$newResponse = new self($com, true, $timeout_s, $timeout_us);
101103
$tagInfo = $reg::parseTag($newResponse->getTag());
102104
$newResponse->setTag($tagInfo[1]);
103105
if (!$reg->add($newResponse, $tagInfo[0])) {
@@ -138,11 +140,19 @@ public function __construct(
138140
*
139141
* @return void
140142
*/
141-
private function _receive(Communicator $com, $asStream = false)
142-
{
143-
if (!$com->getTransmitter()->isAvailable()) {
143+
private function _receive(
144+
Communicator $com,
145+
$asStream = false,
146+
$timeout_s = 0,
147+
$timeout_us = null
148+
) {
149+
if (!$com->getTransmitter()->isDataAwaiting(
150+
$timeout_s,
151+
$timeout_us
152+
)
153+
) {
144154
throw new SocketException(
145-
'Connection aborted by other end. Receiving aborted.',
155+
'No data within the time limit',
146156
50000
147157
);
148158
}

‎tests/ConnectionTest.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -541,8 +541,8 @@ public function testInvalidSocketOnReceive()
541541

542542
new Response($com);
543543
$this->fail('Receiving had to fail.');
544-
} catch (T\SocketException $e) {
545-
$this->assertEquals(4, $e->getCode(), 'Improper exception code.');
544+
} catch (SocketException $e) {
545+
$this->assertEquals(50000, $e->getCode(), 'Improper exception code.');
546546
}
547547
}
548548

@@ -554,8 +554,8 @@ public function testInvalidSocketOnStreamReceive()
554554

555555
new Response($com, true);
556556
$this->fail('Receiving had to fail.');
557-
} catch (T\SocketException $e) {
558-
$this->assertEquals(4, $e->getCode(), 'Improper exception code.');
557+
} catch (SocketException $e) {
558+
$this->assertEquals(50000, $e->getCode(), 'Improper exception code.');
559559
}
560560
}
561561

‎tests/RequestHandlingTest.php‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,11 @@ public function testQuitMessage()
695695

696696
$quitRequest = new Request('/quit');
697697
$quitRequest->send($com);
698-
$quitResponse = new Response($com);
698+
$quitResponse = new Response(
699+
$com,
700+
false,
701+
ini_get('default_socket_timeout')
702+
);
699703
$this->assertEquals(
700704
1,
701705
count($quitResponse->getUnrecognizedWords()),
@@ -716,7 +720,11 @@ public function testQuitMessageStream()
716720

717721
$quitRequest = new Request('/quit');
718722
$quitRequest->send($com);
719-
$quitResponse = new Response($com, true);
723+
$quitResponse = new Response(
724+
$com,
725+
true,
726+
ini_get('default_socket_timeout')
727+
);
720728
$this->assertEquals(
721729
1,
722730
count($quitResponse->getUnrecognizedWords()),
@@ -818,7 +826,11 @@ public function testInvokability()
818826
$this->assertEquals('p', $request->getTag());
819827
$this->assertEquals(array('address' => HOSTNAME), $request());
820828
$request($com);
821-
$response = new Response($com);
829+
$response = new Response(
830+
$com,
831+
false,
832+
ini_get('default_socket_timeout')
833+
);
822834
$this->assertInternalType('array', $response());
823835
$this->assertEquals(HOSTNAME, $response('host'));
824836

@@ -851,7 +863,7 @@ public function testTaglessModePassing()
851863
);
852864
$pingRequest1->send($com1, $reg1);
853865

854-
$response1_1 = new Response($com1, false, $reg1);
866+
$response1_1 = new Response($com1, false, null, null, $reg1);
855867

856868
$cancelRequest = new Request('/cancel');
857869
$reg1->setTaglessMode(true);
@@ -865,9 +877,9 @@ public function testTaglessModePassing()
865877
);
866878
$pingRequest2->send($com2, $reg2);
867879

868-
$response2_1 = new Response($com2, false, $reg2);
869-
$response2_2 = new Response($com2, false, $reg2);
870-
$response2_3 = new Response($com2, false, $reg2);
880+
$response2_1 = new Response($com2, false, null, null, $reg2);
881+
$response2_2 = new Response($com2, false, null, null, $reg2);
882+
$response2_3 = new Response($com2, false, null, null, $reg2);
871883
$reg1->setTaglessMode(false);
872884

873885
$com1->close();
@@ -877,8 +889,8 @@ public function testTaglessModePassing()
877889
$this->assertEquals(Response::TYPE_DATA, $response2_2->getType());
878890
$this->assertEquals(Response::TYPE_FINAL, $response2_3->getType());
879891

880-
$response1_2 = new Response($com1, false, $reg1);
881-
$response1_3 = new Response($com1, false, $reg1);
892+
$response1_2 = new Response($com1, false, null, null, $reg1);
893+
$response1_3 = new Response($com1, false, null, null, $reg1);
882894

883895
$this->assertEquals(Response::TYPE_DATA, $response1_1->getType());
884896
$this->assertEquals(Response::TYPE_ERROR, $response1_2->getType());

0 commit comments

Comments
 (0)