From dev-return-52740-archive-asf-public=cust-asf.ponee.io@thrift.apache.org Sun May 27 22:02:08 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 5C6FC180663 for ; Sun, 27 May 2018 22:02:07 +0200 (CEST) Received: (qmail 91407 invoked by uid 500); 27 May 2018 20:02:06 -0000 Mailing-List: contact dev-help@thrift.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@thrift.apache.org Delivered-To: mailing list dev@thrift.apache.org Received: (qmail 91396 invoked by uid 99); 27 May 2018 20:02:06 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 27 May 2018 20:02:06 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 0ED2018032C for ; Sun, 27 May 2018 20:02:06 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -109.511 X-Spam-Level: X-Spam-Status: No, score=-109.511 tagged_above=-999 required=6.31 tests=[ENV_AND_HDR_SPF_MATCH=-0.5, KAM_ASCII_DIVIDERS=0.8, RCVD_IN_DNSWL_MED=-2.3, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01, USER_IN_DEF_SPF_WL=-7.5, USER_IN_WHITELIST=-100] autolearn=disabled Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id mUaRY0QTmTWn for ; Sun, 27 May 2018 20:02:02 +0000 (UTC) Received: from mailrelay1-us-west.apache.org (mailrelay1-us-west.apache.org [209.188.14.139]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTP id F0E715F405 for ; Sun, 27 May 2018 20:02:00 +0000 (UTC) Received: from jira-lw-us.apache.org (unknown [207.244.88.139]) by mailrelay1-us-west.apache.org (ASF Mail Server at mailrelay1-us-west.apache.org) with ESMTP id 83074E015C for ; Sun, 27 May 2018 20:02:00 +0000 (UTC) Received: from jira-lw-us.apache.org (localhost [127.0.0.1]) by jira-lw-us.apache.org (ASF Mail Server at jira-lw-us.apache.org) with ESMTP id 34D162108B for ; Sun, 27 May 2018 20:02:00 +0000 (UTC) Date: Sun, 27 May 2018 20:02:00 +0000 (UTC) From: "ASF GitHub Bot (JIRA)" To: dev@thrift.apache.org Message-ID: In-Reply-To: References: Subject: [jira] [Commented] (THRIFT-4564) TBufferedTransport can leave corrupt data in the buffer MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394 [ https://issues.apache.org/jira/browse/THRIFT-4564?page=3Dcom.atlassia= n.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D164= 92138#comment-16492138 ]=20 ASF GitHub Bot commented on THRIFT-4564: ---------------------------------------- bforbis opened a new pull request #1558: THRIFT-4564: Reset buffered transp= ort on serialization errors URL: https://github.com/apache/thrift/pull/1558 =20 =20 Tested locally using the following thrift IDL on thrift v0.10.0 ``` struct Foo { 1: string name } =20 service SocketTestService { i16 countFoos(1: list foos) } ``` =20 The following client request will fail on the client side, but leave dat= a on the buffer: ``` client.countFoos([new thriftTypes.Foo(), null, new thriftTypes.Foo()]); ``` =20 Using the new code, the client side error will be caught and the buffere= d transport will be reset, allowing for the socket to be used for a subsequ= ent RPC call without causing a parse error on the server side ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. =20 For queries about this service, please contact Infrastructure at: users@infra.apache.org > TBufferedTransport can leave corrupt data in the buffer > ------------------------------------------------------- > > Key: THRIFT-4564 > URL: https://issues.apache.org/jira/browse/THRIFT-4564 > Project: Thrift > Issue Type: Bug > Components: Node.js - Compiler, Node.js - Library > Affects Versions: 0.10.0, 0.11.0 > Reporter: Brian Forbis > Priority: Minor > > I ran into the following issue on Thrift 0.10.0 using Buffered Transport = and Binary Protocol > IDL File: > {code:java} > struct Foo { > 1: string name > } > service SocketTestService { > i16 countFoos(1: list foos) > } > {code} > Client Call: > {code:java} > async function main() { > numFoos =3D await client.countFoos([ > new thriftTypes.Foo(), > null, > new thriftTypes.Foo(), > ]); > } > {code} > The issue I ran into was a client-side thrown exception: > {code:java} > TypeError: Cannot read property 'write' of null > at SocketTestService_countFoos_args.write (/Users/bforbis/git/bforbis/thr= ift-socket-test/gen10/SocketTestService.js:81:15) > at exports.Client.SocketTestServiceClient.send_countFoos (/Users/bforbis/= git/bforbis/thrift-socket-test/gen10/SocketTestService.js:178:8) > at exports.Client.SocketTestServiceClient.countFoos (/Users/bforbis/git/b= forbis/thrift-socket-test/gen10/SocketTestService.js:165:10) > at main (/Users/bforbis/git/bforbis/thrift-socket-test/client.js:110:28) > at > at process._tickCallback (internal/process/next_tick.js:188:7) > {code} > The cause of the issue is because I pass in *null*=C2=A0as a parameter to= my Foo list, which does not have a write() method to serialize itself when= writing to the buffer. That's okay on its own, but what ends up happening = here is we have *half* of a message still written to our buffered transport= , meaning that a subsequent RPC call on that same transport will fail to be= processed on the server side. > {code:java} > Error: Invalid type: -128 > at TBinaryProtocol.skip (/Users/bforbis/git/bforbis/thrift-socket-test/no= de_modules/thrift/lib/nodejs/lib/thrift/binary_protocol.js:364:13) > at module.exports.Foo.Foo.read (/Users/bforbis/git/bforbis/thrift-socket-= test/gen11/socket-test_types.js:47:15) > at SocketTestService_countFoos_args.read (/Users/bforbis/git/bforbis/thri= ft-socket-test/gen11/SocketTestService.js:51:17) > at exports.Processor.SocketTestServiceProcessor.process_countFoos (/Users= /bforbis/git/bforbis/thrift-socket-test/gen11/SocketTestService.js:224:8) > at exports.Processor.SocketTestServiceProcessor.process (/Users/bforbis/g= it/bforbis/thrift-socket-test/gen11/SocketTestService.js:210:39) > at /Users/bforbis/git/bforbis/thrift-socket-test/node_modules/thrift/lib/= nodejs/lib/thrift/server.js:55:21 > at Socket. (/Users/bforbis/git/bforbis/thrift-socket-test/node= _modules/thrift/lib/nodejs/lib/thrift/buffered_transport.js:48:5) > at emitOne (events.js:121:20) > at Socket.emit (events.js:211:7) > at addChunk (_stream_readable.js:263:12) > {code} > *Proposal*: We should allow applications to have global persistent socket= connections that don't corrupt=C2=A0their buffer if there is a serializati= on error when doing an RPC all on the client. Calls to send_${RPC_NAME} sho= uld be wrapped in a try / catch in case there are client side errors in the= processing code. In the case that an exception is thrown, the buffer in bu= ffered transport should be emptied rather than leaving it full. > *Side Note*:=C2=A0The cause of the client-side exception has been fixed i= n Thrift 0.11.0 in this [Pull Request|https://github.com/apache/thrift/comm= it/de112fbb0d7f2139ef107211e82e03b574f890d0#diff-a686530169a8a14044fae0f95d= 54578c], which casts undef to a new struct. But I fear that this issue wher= e hanging data on the buffered transport isn't cleaned up could come up aga= in. -- This message was sent by Atlassian JIRA (v7.6.3#76005)