thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andy Seaborne (Jira)" <j...@apache.org>
Subject [jira] [Comment Edited] (THRIFT-5022) TIOStreamTransport.isOpen is false for InputStream or Outpstream only use.
Date Thu, 21 Nov 2019 17:14:00 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-5022?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16979323#comment-16979323
] 

Andy Seaborne edited comment on THRIFT-5022 at 11/21/19 5:13 PM:
-----------------------------------------------------------------

This is bringing concerns about error handling – great to consider them but this goes further
than fixing isOpen which is a small regression in 0.13.0 with respect to the two constructors
that take one stream, not two.

Everywhere else IOException is handled and becomes a TTransportException. Transport.close()
does not throw TTransportException.

 

Would it be acceptable to add a try-finally in close so that inputStream and outStream are
set null always? Any non-IOException percolates up?

 [https://github.com/apache/thrift/pull/1942/commits/1afb7359169101c4b685e44421ac4a4533140b8c]

 


was (Author: andy.seaborne):
This is bringing concerns about error handling – great to consider them but this goes further
than fixing isOpen which is a small regression in 0.13.0 with respect to the two constructors
that take one stream, not two.

Everywhere else IOException is handled and becomes a TTransportException. Transport.close()
does not throw TTransportException.

 

Would it be acceptable to add a try-finally in close so that inputStream and outStream are
set null always? Any non-IOException percolates up?

 

 

> TIOStreamTransport.isOpen is false for InputStream or Outpstream only use.
> --------------------------------------------------------------------------
>
>                 Key: THRIFT-5022
>                 URL: https://issues.apache.org/jira/browse/THRIFT-5022
>             Project: Thrift
>          Issue Type: Task
>          Components: Java - Library
>    Affects Versions: 0.13.0
>            Reporter: Andy Seaborne
>            Priority: Major
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> This follows from THRIFT-2530.
> {{TIOStreamTransaport.isOpen}} changed to be
>  
> {noformat}
>   public boolean isOpen() {
>     return inputStream_ != null && outputStream_ != null;
>   }
> {noformat}
> but constructors {{TIOStreamTransaport(InputStream)}} and {{TIOStreamTransaport(OutputStream)}}
leave one of {{inputStream_}} or {{outputStream_}} null.
> This makes isOpen false immediately, no close() called. open() does not change the state
of object.
>  Example:
> {noformat}
> TIOStreamTransport x1 = new TIOStreamTransport(new ByteArrayInputStream(new byte[1]));
>  System.out.println(x1.isOpen());
>  TIOStreamTransport x2 = new TIOStreamTransport(new ByteArrayOutputStream());
>  System.out.println(x2.isOpen());
> {noformat}
> is prints false both times.
>  
> It should be:
> {noformat}
>   public boolean isOpen() {
>     return inputStream_ != null || outputStream_ != null;
>   }
> {noformat}
> or an explicit flag for the open/close state but {{inputStream_ != null || outputStream_
!= null;}} is enough given the current close implementation.
>  
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message