[
https://issues.apache.org/jira/browse/ZOOKEEPER-1246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13142067#comment-13142067
]
Hudson commented on ZOOKEEPER-1246:
-----------------------------------
Integrated in ZooKeeper-trunk #1352 (See
[https://builds.apache.org/job/ZooKeeper-trunk/1352/])
ZOOKEEPER-1246. Dead code in PrepRequestProcessor catch Exception block
(camille)
camille :
http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1196025
Files :
* /zookeeper/trunk/CHANGES.txt
*
/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
*
/zookeeper/trunk/src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
> Dead code in PrepRequestProcessor catch Exception block
> -------------------------------------------------------
>
> Key: ZOOKEEPER-1246
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1246
> Project: ZooKeeper
> Issue Type: Sub-task
> Reporter: Thomas Koch
> Assignee: Camille Fournier
> Priority: Blocker
> Fix For: 3.4.0, 3.5.0
>
> Attachments: ZOOKEEPER-1246.patch, ZOOKEEPER-1246.patch,
> ZOOKEEPER-1246_trunk.patch, ZOOKEEPER-1246_trunk.patch
>
>
> This is a regression introduced by ZOOKEEPER-965 (multi transactions). The
> catch(Exception e) block in PrepRequestProcessor.pRequest contains an if
> block with condition request.getHdr() != null. This condition will always
> evaluate to false since the changes in ZOOKEEPER-965.
> This is caused by a change in sequence: Before ZK-965, the txnHeader was set
> _before_ the deserialization of the request. Afterwards the deserialization
> happens before request.setHdr is set. So the following RequestProcessors
> won't see the request as a failed one but as a Read request, since it doesn't
> have a hdr set.
> Notes:
> - it is very bad practice to catch Exception. The block should rather catch
> IOException
> - The check whether the TxnHeader is set in the request is used at several
> places to see whether the request is a read or write request. It isn't
> obvious for a newby, what it means whether a request has a hdr set or not.
> - at the beginning of pRequest the hdr and txn of request are set to null.
> However there is no chance that these fields could ever not be null at this
> point. The code however suggests that this could be the case. There should
> rather be an assertion that confirms that these fields are indeed null. The
> practice of doing things "just in case", even if there is no chance that this
> case could happen, is a very stinky code smell and means that the code isn't
> understandable or trustworthy.
> - The multi transaction switch case block in pRequest is very hard to read,
> because it missuses the request.{hdr|txn} fields as local variables.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
