[jira] [Commented] (THRIFT-3944) TSSLSocket has dead code in checkHandshake

[James E. King, III (JIRA)]

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

James E. King, III commented on THRIFT-3944:


Looks correct. :+1:

> TSSLSocket has dead code in checkHandshake
> --
>
> Key: THRIFT-3944
> URL: https://issues.apache.org/jira/browse/THRIFT-3944
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>Priority: Minor
>
> There is a block of code in checkHandshake that attempts to set read/write 
> memory bios to be nonblocking. This code doesn't do anything:
> https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L441
> Here's what this code looks like, and the problems:
> - BIO_new(BIO_s_mem()) creates a new memory BIO. Not sure why.
> - BIO_set_nbio() executes BIO_ctrl(..., BIO_C_SET_NBIO, ...). This errors out 
> and return 0 because mem_ctrl does not have a case for BIO_C_SET_NBIO. See: 
> https://github.com/openssl/openssl/blob/6f0ac0e2f27d9240516edb9a23b7863e7ad02898/crypto/bio/bss_mem.c#L226
> - SSL_set_bio() sets the SSL* to use the memory BIOs.
> - SSL_set_fd() creates a socket BIO, sets the FD on it, and uses 
> SSL_set_bio() to replace the memory BIOs.
> As far as I can tell, this block of code does nothing and will not change 
> functionality. If there's a reason that it's there, it needs to be 
> re-implemented.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Comment Edited] (THRIFT-3944) TSSLSocket has dead code in checkHandshake

[James E. King, III (JIRA)]

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

James E. King, III edited comment on THRIFT-3944 at 10/7/16 5:19 AM:
-

Patch looks correct. :+1:


was (Author: jking3):
Looks correct. :+1:

> TSSLSocket has dead code in checkHandshake
> --
>
> Key: THRIFT-3944
> URL: https://issues.apache.org/jira/browse/THRIFT-3944
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>Priority: Minor
>
> There is a block of code in checkHandshake that attempts to set read/write 
> memory bios to be nonblocking. This code doesn't do anything:
> https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L441
> Here's what this code looks like, and the problems:
> - BIO_new(BIO_s_mem()) creates a new memory BIO. Not sure why.
> - BIO_set_nbio() executes BIO_ctrl(..., BIO_C_SET_NBIO, ...). This errors out 
> and return 0 because mem_ctrl does not have a case for BIO_C_SET_NBIO. See: 
> https://github.com/openssl/openssl/blob/6f0ac0e2f27d9240516edb9a23b7863e7ad02898/crypto/bio/bss_mem.c#L226
> - SSL_set_bio() sets the SSL* to use the memory BIOs.
> - SSL_set_fd() creates a socket BIO, sets the FD on it, and uses 
> SSL_set_bio() to replace the memory BIOs.
> As far as I can tell, this block of code does nothing and will not change 
> functionality. If there's a reason that it's there, it needs to be 
> re-implemented.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request #1112: THRIFT-2527: Apache Thrift IDL Compiler code gene...

[bgould]

GitHub user bgould opened a pull request:

https://github.com/apache/thrift/pull/1112

THRIFT-2527: Apache Thrift IDL Compiler code generated for Node.js should 
be jshint clean

Added small style tweaks to generated node.js code so that it passes jshint 
with default options.

When combined with the fix for THRIFT-3546 the node.js code should be able 
to pass with a configuration of:

`{ "strict" : "implied", "node" : "true" }`

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bgould/thrift THRIFT-2527

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1112.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1112


commit f19a407111b42e2b6daf75f1583a61ddb4fd6579
Author: BCG 
Date:   2016-10-06T01:14:18Z

Style tweaks to pass jshint




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-2527) Apache Thrift IDL Compiler code generated for Node.js should be jshint clean

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-2527:


GitHub user bgould opened a pull request:

https://github.com/apache/thrift/pull/1112

THRIFT-2527: Apache Thrift IDL Compiler code generated for Node.js should 
be jshint clean

Added small style tweaks to generated node.js code so that it passes jshint 
with default options.

When combined with the fix for THRIFT-3546 the node.js code should be able 
to pass with a configuration of:

`{ "strict" : "implied", "node" : "true" }`

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bgould/thrift THRIFT-2527

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1112.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1112


commit f19a407111b42e2b6daf75f1583a61ddb4fd6579
Author: BCG 
Date:   2016-10-06T01:14:18Z

Style tweaks to pass jshint




> Apache Thrift IDL Compiler code generated for Node.js should be jshint clean
> 
>
> Key: THRIFT-2527
> URL: https://issues.apache.org/jira/browse/THRIFT-2527
> Project: Thrift
>  Issue Type: Bug
>  Components: Node.js - Compiler
>Affects Versions: 0.9.1
> Environment: all
>Reporter: Randy Abernethy
>
> The Apache Thrift IDL Compiler emits Node.js code which produces numerous 
> JSHint warnings. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3546) NodeJS code should not be namespaced (and is currently not strict-mode compliant)

[Benjamin Gould (JIRA)]

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

Benjamin Gould commented on THRIFT-3546:


The above PR also fixes THRIFT-3835 if I am not mistaken.

> NodeJS code should not be namespaced (and is currently not strict-mode 
> compliant)
> -
>
> Key: THRIFT-3546
> URL: https://issues.apache.org/jira/browse/THRIFT-3546
> Project: Thrift
>  Issue Type: Bug
>  Components: Node.js - Compiler
>Affects Versions: 0.9.3
>Reporter: Yunchi Luo
>
> Code generated for NodeJS, whether with a js namespace specified or not, 
> seems to fail strict mode. Specifically, it doesn't always generate "var" 
> when needed. This might not sound like a big issue but it's quite a pain to 
> deal with...
> It is standard nowadays to enable strict mode for Javascript code 
> (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode).
>  And babel, which is one of most popular ES6 => ES5 transpilers, while does 
> not force strict mode, does seem to break when you don't use "var".
> I think the best solution here is really to just stop using namespaces in 
> node code. Every type will be declared with "var", and if exported, also 
> assigned to "module.exports".
> I don't believe namespaces currently generated in the node code is directly 
> used anywhere or is even accessible outside the file?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3546) NodeJS code should not be namespaced (and is currently not strict-mode compliant)

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-3546:


GitHub user bgould opened a pull request:

https://github.com/apache/thrift/pull/

THRIFT-3546: Remove global namespace objects from nodejs generated code

Modified the node.js code generator to not generate namespace object by 
default, as using require() is the idiomatic way to reference classes contained 
in other modules for node, instead of relying on a object heirarchy existing in 
the global namespace.

This change makes the generated code work under Javascript's "use strict" 
mode.

In case others are relying on the existing behavior, I added a ":with_ns" 
flag to the compiler that enables the current behavior for backwards 
compatibility.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bgould/thrift THRIFT-3546

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #


commit 10d11d733bfec328b94e1a3d70675f67cbc44312
Author: BCG 
Date:   2016-10-05T14:25:27Z

Removed namespace objects from nodejs generated code, and added with_ns 
option for backwards compatibility.




> NodeJS code should not be namespaced (and is currently not strict-mode 
> compliant)
> -
>
> Key: THRIFT-3546
> URL: https://issues.apache.org/jira/browse/THRIFT-3546
> Project: Thrift
>  Issue Type: Bug
>  Components: Node.js - Compiler
>Affects Versions: 0.9.3
>Reporter: Yunchi Luo
>
> Code generated for NodeJS, whether with a js namespace specified or not, 
> seems to fail strict mode. Specifically, it doesn't always generate "var" 
> when needed. This might not sound like a big issue but it's quite a pain to 
> deal with...
> It is standard nowadays to enable strict mode for Javascript code 
> (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode).
>  And babel, which is one of most popular ES6 => ES5 transpilers, while does 
> not force strict mode, does seem to break when you don't use "var".
> I think the best solution here is really to just stop using namespaces in 
> node code. Every type will be declared with "var", and if exported, also 
> assigned to "module.exports".
> I don't believe namespaces currently generated in the node code is directly 
> used anywhere or is even accessible outside the file?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request #1111: THRIFT-3546: Remove global namespace objects from...

[bgould]

GitHub user bgould opened a pull request:

https://github.com/apache/thrift/pull/

THRIFT-3546: Remove global namespace objects from nodejs generated code

Modified the node.js code generator to not generate namespace object by 
default, as using require() is the idiomatic way to reference classes contained 
in other modules for node, instead of relying on a object heirarchy existing in 
the global namespace.

This change makes the generated code work under Javascript's "use strict" 
mode.

In case others are relying on the existing behavior, I added a ":with_ns" 
flag to the compiler that enables the current behavior for backwards 
compatibility.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bgould/thrift THRIFT-3546

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #


commit 10d11d733bfec328b94e1a3d70675f67cbc44312
Author: BCG 
Date:   2016-10-05T14:25:27Z

Removed namespace objects from nodejs generated code, and added with_ns 
option for backwards compatibility.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3546) NodeJS code should not be namespaced (and is currently not strict-mode compliant)

[Benjamin Gould (JIRA)]

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

Benjamin Gould commented on THRIFT-3546:


I have a fix for this that eliminates the namespace object heirarchy in node.js 
generated code, and adds a compiler flag that can be used for backwards 
compatibility if necessary.

I think that not generating the namespace objects for node.js/browserify is a 
sensible default behavior because for node.js/browserify it is idiomatic to use 
require() rather than relying on objects being in the global namespace (which 
makes me doubt that very many developers if any are relying on that behavior).

Github PR coming shortly.

> NodeJS code should not be namespaced (and is currently not strict-mode 
> compliant)
> -
>
> Key: THRIFT-3546
> URL: https://issues.apache.org/jira/browse/THRIFT-3546
> Project: Thrift
>  Issue Type: Bug
>  Components: Node.js - Compiler
>Affects Versions: 0.9.3
>Reporter: Yunchi Luo
>
> Code generated for NodeJS, whether with a js namespace specified or not, 
> seems to fail strict mode. Specifically, it doesn't always generate "var" 
> when needed. This might not sound like a big issue but it's quite a pain to 
> deal with...
> It is standard nowadays to enable strict mode for Javascript code 
> (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode).
>  And babel, which is one of most popular ES6 => ES5 transpilers, while does 
> not force strict mode, does seem to break when you don't use "var".
> I think the best solution here is really to just stop using namespaces in 
> node code. Every type will be declared with "var", and if exported, also 
> assigned to "module.exports".
> I don't believe namespaces currently generated in the node code is directly 
> used anywhere or is even accessible outside the file?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3410) Support for 'use strict' in generated javascript code

[Benjamin Gould (JIRA)]

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

Benjamin Gould commented on THRIFT-3410:


THRIFT-3546 solves this for node.js/browserify, likely still broken for 
Typescript and browser JS however

> Support for 'use strict' in generated javascript code
> -
>
> Key: THRIFT-3410
> URL: https://issues.apache.org/jira/browse/THRIFT-3410
> Project: Thrift
>  Issue Type: Improvement
>  Components: JavaScript - Compiler
>Affects Versions: 0.9.3
>Reporter: Mateus Chagas Sousa
>Priority: Minor
>  Labels: easyfix, javascript, js, node.js, strict, var
>
> Generated structs in javascript aren't compatible with strict mode cause they 
> are declared as global variables instead of being declared using the _var_ 
> keyword.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3944) TSSLSocket has dead code in checkHandshake

[Aki Sukegawa (JIRA)]

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

Aki Sukegawa commented on THRIFT-3944:
--

The patch looks correct.

> If there's a reason that it's there, it needs to be re-implemented.

The code comment says that it is setting BIOs to non-blocking.
Since the socket is set to non-blocking right before this and the BIOs created 
by SSL_set_fd inherit the socket's non-blocking behavior, I don't think there's 
something we need to re-implement.


> TSSLSocket has dead code in checkHandshake
> --
>
> Key: THRIFT-3944
> URL: https://issues.apache.org/jira/browse/THRIFT-3944
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>Priority: Minor
>
> There is a block of code in checkHandshake that attempts to set read/write 
> memory bios to be nonblocking. This code doesn't do anything:
> https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L441
> Here's what this code looks like, and the problems:
> - BIO_new(BIO_s_mem()) creates a new memory BIO. Not sure why.
> - BIO_set_nbio() executes BIO_ctrl(..., BIO_C_SET_NBIO, ...). This errors out 
> and return 0 because mem_ctrl does not have a case for BIO_C_SET_NBIO. See: 
> https://github.com/openssl/openssl/blob/6f0ac0e2f27d9240516edb9a23b7863e7ad02898/crypto/bio/bss_mem.c#L226
> - SSL_set_bio() sets the SSL* to use the memory BIOs.
> - SSL_set_fd() creates a socket BIO, sets the FD on it, and uses 
> SSL_set_bio() to replace the memory BIOs.
> As far as I can tell, this block of code does nothing and will not change 
> functionality. If there's a reason that it's there, it needs to be 
> re-implemented.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3944) TSSLSocket has dead code in checkHandshake

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-3944:


GitHub user tpcwang opened a pull request:

https://github.com/apache/thrift/pull/1110

THRIFT-3944 TSSLSocket has dead code in checkHandshake

See https://issues.apache.org/jira/browse/THRIFT-3944 for why I think this 
can be safely removed without changing functionality.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/tpcwang/thrift THRIFT-3944

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1110.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1110


commit fe26d8ad37f82a5a8231e8ab3f0b794912b1eb99
Author: tpcwang 
Date:   2016-10-06T17:41:44Z

THRIFT-3944 TSSLSocket has dead code in checkHandshake




> TSSLSocket has dead code in checkHandshake
> --
>
> Key: THRIFT-3944
> URL: https://issues.apache.org/jira/browse/THRIFT-3944
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>Priority: Minor
>
> There is a block of code in checkHandshake that attempts to set read/write 
> memory bios to be nonblocking. This code doesn't do anything:
> https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L441
> Here's what this code looks like, and the problems:
> - BIO_new(BIO_s_mem()) creates a new memory BIO. Not sure why.
> - BIO_set_nbio() executes BIO_ctrl(..., BIO_C_SET_NBIO, ...). This errors out 
> and return 0 because mem_ctrl does not have a case for BIO_C_SET_NBIO. See: 
> https://github.com/openssl/openssl/blob/6f0ac0e2f27d9240516edb9a23b7863e7ad02898/crypto/bio/bss_mem.c#L226
> - SSL_set_bio() sets the SSL* to use the memory BIOs.
> - SSL_set_fd() creates a socket BIO, sets the FD on it, and uses 
> SSL_set_bio() to replace the memory BIOs.
> As far as I can tell, this block of code does nothing and will not change 
> functionality. If there's a reason that it's there, it needs to be 
> re-implemented.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift pull request #1110: THRIFT-3944 TSSLSocket has dead code in checkHand...

[tpcwang]

GitHub user tpcwang opened a pull request:

https://github.com/apache/thrift/pull/1110

THRIFT-3944 TSSLSocket has dead code in checkHandshake

See https://issues.apache.org/jira/browse/THRIFT-3944 for why I think this 
can be safely removed without changing functionality.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/tpcwang/thrift THRIFT-3944

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1110.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1110


commit fe26d8ad37f82a5a8231e8ab3f0b794912b1eb99
Author: tpcwang 
Date:   2016-10-06T17:41:44Z

THRIFT-3944 TSSLSocket has dead code in checkHandshake




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Created] (THRIFT-3944) TSSLSocket has dead code in checkHandshake

[Ted Wang (JIRA)]

Ted Wang created THRIFT-3944:


 Summary: TSSLSocket has dead code in checkHandshake
 Key: THRIFT-3944
 URL: https://issues.apache.org/jira/browse/THRIFT-3944
 Project: Thrift
  Issue Type: Bug
  Components: C++ - Library
Affects Versions: 0.9.3
Reporter: Ted Wang
Assignee: Ted Wang
Priority: Minor


There is a block of code in checkHandshake that attempts to set read/write 
memory bios to be nonblocking. This code doesn't do anything:
https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L441

Here's what this code looks like, and the problems:
- BIO_new(BIO_s_mem()) creates a new memory BIO. Not sure why.
- BIO_set_nbio() executes BIO_ctrl(..., BIO_C_SET_NBIO, ...). This errors out 
and return 0 because mem_ctrl does not have a case for BIO_C_SET_NBIO. See: 
https://github.com/openssl/openssl/blob/6f0ac0e2f27d9240516edb9a23b7863e7ad02898/crypto/bio/bss_mem.c#L226
- SSL_set_bio() sets the SSL* to use the memory BIOs.
- SSL_set_fd() creates a socket BIO, sets the FD on it, and uses SSL_set_bio() 
to replace the memory BIOs.

As far as I can tell, this block of code does nothing and will not change 
functionality. If there's a reason that it's there, it needs to be 
re-implemented.




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3942) TSSLSocket does not honor send and receive timeouts

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-3942:


Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1108
  
Looks good to me. :+1:


> TSSLSocket does not honor send and receive timeouts
> ---
>
> Key: THRIFT-3942
> URL: https://issues.apache.org/jira/browse/THRIFT-3942
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> TSocket respects send and receive timeout, but TSSLSocket does not. This is 
> because it always pass -1 to THRIFT_POLL in waitForEvent:
> https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L645



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift issue #1108: THRIFT-3942 Make TSSLSocket honor send and receive timeo...

[jeking3]

Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1108
  
Looks good to me. :+1:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1108: THRIFT-3942 Make TSSLSocket honor send and receiv...

[tpcwang]

Github user tpcwang commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1108#discussion_r82233502
  
--- Diff: lib/cpp/test/TSSLSocketInterruptTest.cpp ---
@@ -57,12 +54,6 @@ struct GlobalFixtureSSL
 BOOST_TEST_MESSAGE(boost::format("argv[%1%] = \"%2%\"") % i % 
master_test_suite().argv[i]);
   }
 
-#ifdef __linux__
-  // OpenSSL calls send() without MSG_NOSIGPIPE so writing to a socket 
that has
-   // disconnected can cause a SIGPIPE signal...
-   signal(SIGPIPE, SIG_IGN);
--- End diff --

This is part of the test code for setup/teardown, so it should not affect 
applications, but I've reverted my change here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3942) TSSLSocket does not honor send and receive timeouts

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-3942:


Github user tpcwang commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1108#discussion_r82233502
  
--- Diff: lib/cpp/test/TSSLSocketInterruptTest.cpp ---
@@ -57,12 +54,6 @@ struct GlobalFixtureSSL
 BOOST_TEST_MESSAGE(boost::format("argv[%1%] = \"%2%\"") % i % 
master_test_suite().argv[i]);
   }
 
-#ifdef __linux__
-  // OpenSSL calls send() without MSG_NOSIGPIPE so writing to a socket 
that has
-   // disconnected can cause a SIGPIPE signal...
-   signal(SIGPIPE, SIG_IGN);
--- End diff --

This is part of the test code for setup/teardown, so it should not affect 
applications, but I've reverted my change here.


> TSSLSocket does not honor send and receive timeouts
> ---
>
> Key: THRIFT-3942
> URL: https://issues.apache.org/jira/browse/THRIFT-3942
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> TSocket respects send and receive timeout, but TSSLSocket does not. This is 
> because it always pass -1 to THRIFT_POLL in waitForEvent:
> https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L645



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-2794) generated Java code full of warnings

[James E. King, III (JIRA)]

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

James E. King, III commented on THRIFT-2794:


It looks like changes were made relating to this defect; is it resolved and if 
so in what version (since code was merged a year ago)?

> generated Java code full of warnings
> 
>
> Key: THRIFT-2794
> URL: https://issues.apache.org/jira/browse/THRIFT-2794
> Project: Thrift
>  Issue Type: Bug
>  Components: Java - Compiler
>Affects Versions: 0.9.1, 1.0
> Environment: Ubuntu 14.04
>Reporter: Kevin
>Priority: Minor
>
> The java code generated by thrift is full of warnings about unused imports 
> and fields. With a lot of thrift generated classes, we're getting warnings we 
> care about drowned out in the noise of generated code warnings (approx. 600).
> This small example generates about a half dozen unused imports:
> struct Publisher
> {
>   1: optional string id,
>   2: optional string name,
>   3: optional list cat,
>   4: optional string domain
> }
> thrift --gen java -out . publisher.thrift 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-2564) misc. Lua issues reported by coverity scan

[James E. King, III (JIRA)]

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

James E. King, III commented on THRIFT-2564:


Some of these were taken care of in THRIFT-3943.
We should rescan after that is merged and see what's left.

> misc. Lua issues reported by coverity scan
> --
>
> Key: THRIFT-2564
> URL: https://issues.apache.org/jira/browse/THRIFT-2564
> Project: Thrift
>  Issue Type: Bug
>  Components: Lua - Library
>Affects Versions: 0.9.2, 0.9.3
>Reporter: Jens Geyer
>
> ** CID 1216829:  Unchecked return value from library  (CHECKED_RETURN)
> /lib/lua/src/usocket.c: 255 in socket_setnonblocking()
> ** CID 1216828:  Unchecked return value from library  (CHECKED_RETURN)
> /lib/lua/src/usocket.c: 261 in socket_setblocking()
> ** CID 1216832:  Logically dead code  (DEADCODE)
> /lib/lua/src/usocket.c: 166 in socket_accept()
> ** CID 1216831:  Logically dead code  (DEADCODE)
> /lib/lua/src/usocket.c: 244 in socket_recv()
> ** CID 1216830:  Logically dead code  (DEADCODE)
> /lib/lua/src/usocket.c: 216 in socket_send()
> ** CID 1216838:  Resource leak  (RESOURCE_LEAK)
> /lib/lua/src/luasocket.c: 316 in l_socket_accept()
> ** CID 1216834:  Division or modulo by zero  (DIVIDE_BY_ZERO)
> /lib/lua/src/lualongnumber.c: 85 in l_div()
> ** CID 1216833:  Division or modulo by zero  (DIVIDE_BY_ZERO)
> /lib/lua/src/lualongnumber.c: 129 in l_mod()
> ** CID 1216844:  Unused pointer value  (UNUSED_VALUE)
> /lib/lua/src/luasocket.c: 233 in l_socket_send()
> ** CID 1216837:  Resource leak  (RESOURCE_LEAK)
> /lib/lua/src/luasocket.c: 221 in l_socket_create()
> ** CID 1216836:  Resource leak  (RESOURCE_LEAK)
> /lib/lua/src/luasocket.c: 372 in l_socket_create_and_connect()
> ** CID 1216842:  Uninitialized scalar variable  (UNINIT)
> /lib/lua/src/usocket.c: 80 in socket_wait()
> ** CID 1216841:  Uninitialized scalar variable  (UNINIT)
> /lib/lua/src/usocket.c: 80 in socket_wait()
> ** CID 1216840:  Uninitialized scalar variable  (UNINIT)
> /lib/lua/src/usocket.c: 136 in socket_get_info()



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Updated] (THRIFT-2564) misc. Lua issues reported by coverity scan

[James E. King, III (JIRA)]

 [ 
https://issues.apache.org/jira/browse/THRIFT-2564?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

James E. King, III updated THRIFT-2564:
---
Affects Version/s: 0.9.3

> misc. Lua issues reported by coverity scan
> --
>
> Key: THRIFT-2564
> URL: https://issues.apache.org/jira/browse/THRIFT-2564
> Project: Thrift
>  Issue Type: Bug
>  Components: Lua - Library
>Affects Versions: 0.9.2, 0.9.3
>Reporter: Jens Geyer
>
> ** CID 1216829:  Unchecked return value from library  (CHECKED_RETURN)
> /lib/lua/src/usocket.c: 255 in socket_setnonblocking()
> ** CID 1216828:  Unchecked return value from library  (CHECKED_RETURN)
> /lib/lua/src/usocket.c: 261 in socket_setblocking()
> ** CID 1216832:  Logically dead code  (DEADCODE)
> /lib/lua/src/usocket.c: 166 in socket_accept()
> ** CID 1216831:  Logically dead code  (DEADCODE)
> /lib/lua/src/usocket.c: 244 in socket_recv()
> ** CID 1216830:  Logically dead code  (DEADCODE)
> /lib/lua/src/usocket.c: 216 in socket_send()
> ** CID 1216838:  Resource leak  (RESOURCE_LEAK)
> /lib/lua/src/luasocket.c: 316 in l_socket_accept()
> ** CID 1216834:  Division or modulo by zero  (DIVIDE_BY_ZERO)
> /lib/lua/src/lualongnumber.c: 85 in l_div()
> ** CID 1216833:  Division or modulo by zero  (DIVIDE_BY_ZERO)
> /lib/lua/src/lualongnumber.c: 129 in l_mod()
> ** CID 1216844:  Unused pointer value  (UNUSED_VALUE)
> /lib/lua/src/luasocket.c: 233 in l_socket_send()
> ** CID 1216837:  Resource leak  (RESOURCE_LEAK)
> /lib/lua/src/luasocket.c: 221 in l_socket_create()
> ** CID 1216836:  Resource leak  (RESOURCE_LEAK)
> /lib/lua/src/luasocket.c: 372 in l_socket_create_and_connect()
> ** CID 1216842:  Uninitialized scalar variable  (UNINIT)
> /lib/lua/src/usocket.c: 80 in socket_wait()
> ** CID 1216841:  Uninitialized scalar variable  (UNINIT)
> /lib/lua/src/usocket.c: 80 in socket_wait()
> ** CID 1216840:  Uninitialized scalar variable  (UNINIT)
> /lib/lua/src/usocket.c: 136 in socket_get_info()



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift issue #1106: THRIFT-3868 Java struct equals should do identity check ...

[jeking3]

Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1106
  
Looks good to me.  :+1:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[jira] [Commented] (THRIFT-3868) Java struct equals should do identity check before field comparison

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-3868:


Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1106
  
Looks good to me.  :+1:


> Java struct equals should do identity check before field comparison
> ---
>
> Key: THRIFT-3868
> URL: https://issues.apache.org/jira/browse/THRIFT-3868
> Project: Thrift
>  Issue Type: Improvement
>  Components: Java - Compiler
>Affects Versions: 0.9.3, 0.10.0
>Reporter: Mike Rettig
>Priority: Minor
>
> The identity check is cheap and should be done before comparing fields of a 
> struct. Idiomatic equals methods always include this check especially if the 
> field by field comparison can be expensive. 
> Check to add:
> if(that == this) return true;
> 1864   out << indent() << "public boolean equals(" << tstruct->get_name() << 
> " that) {" << endl;
> 1865   indent_up();
> 1866   out << indent() << "if (that == null)" << endl << indent() << "  
> return false;" << endl;
> INSERT IDENTITY CHECK HERE
> 1867 
> 1868   const vector& members = tstruct->get_members();
> 1869   vector::const_iterator m_iter;
> 1870   for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
> 1871 out << endl;
> 1872 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Updated] (THRIFT-3943) Coverity Scan identified some high severity defects

[James E. King, III (JIRA)]

 [ 
https://issues.apache.org/jira/browse/THRIFT-3943?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

James E. King, III updated THRIFT-3943:
---
Description: 
Coverity Scan identified 9 issues of high severity.
I dismissed 4 of them as false positives; coverity lost track of the handling 
of socket file descriptors across multiple layers of calls; this left 5 issues, 
and I took care of a number of insignificant issues as well:

1295822 - memory leak in ThreadFactoryTests
1216842 - uninitialized rfds fd_set is passed to select if mode is not 
WAIT_MODE_C (R+W)
1216841 - uninitialized wfds fd_set is passed to select if mode is not 
WAIT_MODE_C (R+W)
1216840 - getsockname is always passed uninitialized addrlen
1295810 - uninitialized variables in test
1295808 - uninitialized variable in test
1295804 - structurally dead code in processor test event log - changed to use 
environment variable
excuded:
1174563 - memory leak in compiler class handling functions
1174671 - uninitialized variable in FunctionRunner (intervalMs_)
1174669, 1174763, 1295806, 1295807, 1295809 - uninitialized variable in TSocket 
(peerPort_)

  was:
Coverity Scan identified 9 issues of high severity.
I dismissed 4 of them as false positives; coverity lost track of the handling 
of socket file descriptors across multiple layers of calls; this left 5 issues:

1295822 - memory leak in ThreadFactoryTests
1216842 - uninitialized rfds fd_set is passed to select if mode is not 
WAIT_MODE_C (R+W)
1216841 - uninitialized wfds fd_set is passed to select if mode is not 
WAIT_MODE_C (R+W)
1216840 - getsockname is always passed uninitialized addrlen
1174563 - memory leak in compiler class handling functions


> Coverity Scan identified some high severity defects
> ---
>
> Key: THRIFT-3943
> URL: https://issues.apache.org/jira/browse/THRIFT-3943
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library, Lua - Library
>Affects Versions: 0.9.3
> Environment: https://scan.coverity.com/projects/thrift
>Reporter: James E. King, III
>Assignee: James E. King, III
>Priority: Critical
>
> Coverity Scan identified 9 issues of high severity.
> I dismissed 4 of them as false positives; coverity lost track of the handling 
> of socket file descriptors across multiple layers of calls; this left 5 
> issues, and I took care of a number of insignificant issues as well:
> 1295822 - memory leak in ThreadFactoryTests
> 1216842 - uninitialized rfds fd_set is passed to select if mode is not 
> WAIT_MODE_C (R+W)
> 1216841 - uninitialized wfds fd_set is passed to select if mode is not 
> WAIT_MODE_C (R+W)
> 1216840 - getsockname is always passed uninitialized addrlen
> 1295810 - uninitialized variables in test
> 1295808 - uninitialized variable in test
> 1295804 - structurally dead code in processor test event log - changed to use 
> environment variable
> excuded:
> 1174563 - memory leak in compiler class handling functions
> 1174671 - uninitialized variable in FunctionRunner (intervalMs_)
> 1174669, 1174763, 1295806, 1295807, 1295809 - uninitialized variable in 
> TSocket (peerPort_)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-3943) Coverity Scan identified some high severity defects

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-3943:


Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1109
  
I backed out the delete _xcception change in the compiler (1174563 - memory 
leak in compiler class handling functions), we'll see if that was the culprit.


> Coverity Scan identified some high severity defects
> ---
>
> Key: THRIFT-3943
> URL: https://issues.apache.org/jira/browse/THRIFT-3943
> Project: Thrift
>  Issue Type: Bug
>  Components: C++ - Library, Lua - Library
>Affects Versions: 0.9.3
> Environment: https://scan.coverity.com/projects/thrift
>Reporter: James E. King, III
>Assignee: James E. King, III
>Priority: Critical
>
> Coverity Scan identified 9 issues of high severity.
> I dismissed 4 of them as false positives; coverity lost track of the handling 
> of socket file descriptors across multiple layers of calls; this left 5 
> issues:
> 1295822 - memory leak in ThreadFactoryTests
> 1216842 - uninitialized rfds fd_set is passed to select if mode is not 
> WAIT_MODE_C (R+W)
> 1216841 - uninitialized wfds fd_set is passed to select if mode is not 
> WAIT_MODE_C (R+W)
> 1216840 - getsockname is always passed uninitialized addrlen
> 1174563 - memory leak in compiler class handling functions



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] thrift issue #1109: THRIFT-3943: resolve some high severity outstanding defe...

[jeking3]

Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1109
  
I backed out the delete _xcception change in the compiler (1174563 - memory 
leak in compiler class handling functions), we'll see if that was the culprit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---