Repository: thrift Updated Branches: refs/heads/master 567df43e8 -> 199833807
THRIFT-1579 PHP Extension - function thrift_protocol_read_binary not working from TBinarySerializer::deserialize Patch: Tobias Heintz Project: http://git-wip-us.apache.org/repos/asf/thrift/repo Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/19983380 Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/19983380 Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/19983380 Branch: refs/heads/master Commit: 199833807f8dabd0d6d1707a594b7d6cac82641e Parents: 567df43 Author: Jens Geyer <[email protected]> Authored: Sat Feb 22 17:34:29 2014 +0100 Committer: Jens Geyer <[email protected]> Committed: Sat Feb 22 17:34:29 2014 +0100 ---------------------------------------------------------------------- .../Protocol/TBinaryProtocolAccelerated.php | 17 ++++- .../lib/Thrift/Serializer/TBinarySerializer.php | 8 ++- lib/php/test/Makefile.am | 1 + .../Thrift/Protocol/TestBinarySerializer.php | 65 ++++++++++++++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/thrift/blob/19983380/lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php ---------------------------------------------------------------------- diff --git a/lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php b/lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php index 392aa21..7a40ce9 100644 --- a/lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php +++ b/lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php @@ -33,6 +33,21 @@ class TBinaryProtocolAccelerated extends TBinaryProtocol { public function __construct($trans, $strictRead=false, $strictWrite=true) { // If the transport doesn't implement putBack, wrap it in a // TBufferedTransport (which does) + + // NOTE (t.heintz): This is very evil to do, because the TBufferedTransport may swallow bytes, which + // are then never written to the underlying transport. This happens precisely when a number of bytes + // less than the max buffer size (512 by default) is written to the transport and then flush() is NOT + // called. In that case the data stays in the writeBuffer of the transport, from where it can never be + // accessed again (for example through read()). + // + // Since the caller of this method does not know about the wrapping transport, this creates bugs which + // are very difficult to find. Hence the wrapping of a transport in a buffer should be left to the + // calling code. An interface could used to mandate the presence of the putBack() method in the transport. + // + // I am leaving this code in nonetheless, because there may be applications depending on this behavior. + // + // @see THRIFT-1579 + if (!method_exists($trans, 'putBack')) { $trans = new TBufferedTransport($trans); } @@ -44,4 +59,4 @@ class TBinaryProtocolAccelerated extends TBinaryProtocol { public function isStrictWrite() { return $this->strictWrite_; } -} \ No newline at end of file +} http://git-wip-us.apache.org/repos/asf/thrift/blob/19983380/lib/php/lib/Thrift/Serializer/TBinarySerializer.php ---------------------------------------------------------------------- diff --git a/lib/php/lib/Thrift/Serializer/TBinarySerializer.php b/lib/php/lib/Thrift/Serializer/TBinarySerializer.php index 2a7cc3e..4e0af87 100644 --- a/lib/php/lib/Thrift/Serializer/TBinarySerializer.php +++ b/lib/php/lib/Thrift/Serializer/TBinarySerializer.php @@ -59,8 +59,14 @@ class TBinarySerializer { $transport = new TMemoryBuffer(); $protocol = new TBinaryProtocolAccelerated($transport); if (function_exists('thrift_protocol_read_binary')) { + // NOTE (t.heintz) TBinaryProtocolAccelerated internally wraps our TMemoryBuffer in a + // TBufferedTransport, so we have to retrieve it again or risk losing data when writing + // less than 512 bytes to the transport (see the comment there as well). + // @see THRIFT-1579 $protocol->writeMessageBegin('', TMessageType::REPLY, 0); - $transport->write($string_object); + $protocolTransport = $protocol->getTransport(); + $protocolTransport->write($string_object); + $protocolTransport->flush(); return thrift_protocol_read_binary($protocol, $class_name, $protocol->isStrictRead()); } else { http://git-wip-us.apache.org/repos/asf/thrift/blob/19983380/lib/php/test/Makefile.am ---------------------------------------------------------------------- diff --git a/lib/php/test/Makefile.am b/lib/php/test/Makefile.am index 2fd7f81..1292b81 100755 --- a/lib/php/test/Makefile.am +++ b/lib/php/test/Makefile.am @@ -26,6 +26,7 @@ stubs: ../../../test/ThriftTest.thrift if HAVE_PHPUNIT check: stubs $(PHPUNIT) --log-junit=phpunit.xml Test/Thrift/Protocol/TestTJSONProtocol.php + $(PHPUNIT) --log-junit=phpunit.xml Test/Thrift/Protocol/TestBinarySerializer.php endif clean-local: http://git-wip-us.apache.org/repos/asf/thrift/blob/19983380/lib/php/test/Test/Thrift/Protocol/TestBinarySerializer.php ---------------------------------------------------------------------- diff --git a/lib/php/test/Test/Thrift/Protocol/TestBinarySerializer.php b/lib/php/test/Test/Thrift/Protocol/TestBinarySerializer.php new file mode 100644 index 0000000..65feadd --- /dev/null +++ b/lib/php/test/Test/Thrift/Protocol/TestBinarySerializer.php @@ -0,0 +1,65 @@ +<?php + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + * @package thrift.test + */ + +namespace test\Thrift\Protocol; + +use Thrift\ClassLoader\ThriftClassLoader; +use Thrift\Serializer\TBinarySerializer; + +require_once __DIR__.'/../../../../lib/Thrift/ClassLoader/ThriftClassLoader.php'; + +$loader = new ThriftClassLoader(); +$loader->registerNamespace('Thrift', __DIR__ . '/../../../../lib'); +$loader->registerNamespace('Test', __DIR__ . '/../../..'); +$loader->registerDefinition('ThriftTest', __DIR__ . '/../../../packages'); +$loader->register(); + +/*** + * This test suite depends on running the compiler against the + * standard ThriftTest.thrift file: + * + * lib/php/test$ ../../../compiler/cpp/thrift --gen php -r \ + * --out ./packages ../../../test/ThriftTest.thrift + */ + +class TestBinarySerializer extends \PHPUnit_Framework_TestCase +{ + + public function setUp() + { + } + + /** + * We try to serialize and deserialize a random object to make sure no exceptions are thrown. + * @see THRIFT-1579 + */ + public function testBinarySerializer() + { + $struct = new \ThriftTest\Xtruct(array('string_thing' => 'abc')); + $serialized = TBinarySerializer::serialize($struct, 'ThriftTest\\Xtruct'); + $deserialized = TBinarySerializer::deserialize($serialized, 'ThriftTest\\Xtruct'); + $this->assertEquals($struct, $deserialized); + } + +} +
