You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "alexandre parenteau (JIRA)" <ji...@apache.org> on 2011/07/24 12:04:10 UTC
[jira] [Created] (THRIFT-1243) TAsyncChannel callbacks
TAsyncChannel callbacks
-----------------------
Key: THRIFT-1243
URL: https://issues.apache.org/jira/browse/THRIFT-1243
Project: Thrift
Issue Type: Improvement
Components: C++ - Library
Affects Versions: 0.7
Environment: Visual C++ 2010
Reporter: alexandre parenteau
Priority: Minor
Fix For: 0.8
[Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
1. the returned value is never checked
2. they always return true
Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Assigned] (THRIFT-1243) TAsyncChannel callbacks
Posted by "alexandre parenteau (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
alexandre parenteau reassigned THRIFT-1243:
-------------------------------------------
Assignee: Roger Meier (was: alexandre parenteau)
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: Roger Meier
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243-2.patch, THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1243) TAsyncChannel callbacks
Posted by "alexandre parenteau (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13096614#comment-13096614 ]
alexandre parenteau commented on THRIFT-1243:
---------------------------------------------
@diwaker: Thanks for your comments, I share your concerns and am working currently on a patch for 0.8 that will use exception handling, and more generally handle/propagate libevent errors, log errors... For example I'll propose to remove 'abort' calls in TEvhttpClientChannel.cpp TEvhttpServer.cpp, and replace them by exceptions. Also one significant issue I plan to cover, is to avoid propagating exceptions to libevent (which is C code): see THRIFT-1222 for example.
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1243) TAsyncChannel callbacks
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13102248#comment-13102248 ]
Hudson commented on THRIFT-1243:
--------------------------------
Integrated in Thrift #257 (See [https://builds.apache.org/job/Thrift/257/])
THRIFT-1243 TAsyncChannel callbacks
improved exception handling
Patch: Alexandre Parenteau
roger : http://svn.apache.org/viewvc/?view=rev&rev=1167679
Files :
* /thrift/trunk/lib/cpp/src/async/TEvhttpClientChannel.cpp
* /thrift/trunk/lib/cpp/src/async/TEvhttpServer.cpp
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243-2.patch, THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Reopened] (THRIFT-1243) TAsyncChannel callbacks
Posted by "alexandre parenteau (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
alexandre parenteau reopened THRIFT-1243:
-----------------------------------------
Re-opening with new patch for trunk (0.8)
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243-2.patch, THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (THRIFT-1243) TAsyncChannel callbacks
Posted by "alexandre parenteau (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
alexandre parenteau updated THRIFT-1243:
----------------------------------------
Attachment: THRIFT-1243.patch
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1243) TAsyncChannel callbacks
Posted by "Diwaker Gupta (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13093043#comment-13093043 ]
Diwaker Gupta commented on THRIFT-1243:
---------------------------------------
Saw this a bit late but I'm concerned about this change. With this change, there is no way for implementors of sendMessage/recvMessage to signal to the callers that something unexpected happened or any other error conditions. Just because the current implementation ignores the return values doesn't mean they're not useful.
Is the recommendation now to throw a TException instead?
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1243) TAsyncChannel callbacks
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13072976#comment-13072976 ]
Hudson commented on THRIFT-1243:
--------------------------------
Integrated in Thrift #195 (See [https://builds.apache.org/job/Thrift/195/])
THRIFT-1243 TAsyncChannel callbacks (use void instead of bool)
Patch: Alexandre Parenteau
roger : http://svn.apache.org/viewvc/?view=rev&rev=1151940
Files :
* /thrift/trunk/lib/cpp/src/async/TEvhttpClientChannel.cpp
* /thrift/trunk/lib/cpp/src/async/TAsyncChannel.cpp
* /thrift/trunk/lib/cpp/src/async/TEvhttpClientChannel.h
* /thrift/trunk/lib/cpp/src/async/TAsyncChannel.h
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (THRIFT-1243) TAsyncChannel callbacks
Posted by "alexandre parenteau (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
alexandre parenteau updated THRIFT-1243:
----------------------------------------
Attachment: THRIFT-1243-2.patch
Included is a patch on 0.8 that adds more error handling to the libevent based server.
It also fixes 2 minor bugs (handle failure of evhttp_bind_socket and prevent exceptions to reach libevent in TEvhttpClientChannel::response).
As far as I can tell, the question of @diwaker is still valid, there is no good way currently to have the callback of TEvhttpClientChannel::finish, to be passed the exact failure. However I don't think returning a bool helps, because libevent is the one asynchronously calling, so the error will have to be handled by the client *within* the callback.
This callback will invariably throw a TTransportException::END_OF_FILE, when a disconnect or a 404 happens.
I added to this patch the ability to log such failures to stderr, but it would take more changes (compiler?) to actually pass the error to the client, so that a call will throw the correct error (and *not* EOF).
Finally I removed all abort calls, trying to replace them by appropriate exceptions instead.
Thanks!
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243-2.patch, THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (THRIFT-1243) TAsyncChannel callbacks
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Roger Meier resolved THRIFT-1243.
---------------------------------
Resolution: Fixed
Assignee: alexandre parenteau (was: Roger Meier)
Thanks Alexandre, committed!
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243-2.patch, THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (THRIFT-1243) TAsyncChannel callbacks
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1243?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Roger Meier resolved THRIFT-1243.
---------------------------------
Resolution: Fixed
Assignee: alexandre parenteau
This makes sense, just committed your patch.
Thanks Alexandre
> TAsyncChannel callbacks
> -----------------------
>
> Key: THRIFT-1243
> URL: https://issues.apache.org/jira/browse/THRIFT-1243
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.7
> Environment: Visual C++ 2010
> Reporter: alexandre parenteau
> Assignee: alexandre parenteau
> Priority: Minor
> Labels: patch
> Fix For: 0.8
>
> Attachments: THRIFT-1243.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> [Context: This patch is part of a larger patch to use thriftnb on Windows/VisualC++. See https://github.com/aubonbeurre/thrift/blob/alex-0.7.0/README.non.blocking.Windows for more details.]
> When compiling using Visual C++ 2010, the compiler chokes on casting some callbacks bool(*)() to void(*)().
> Although probably valid and supported by gcc, this is further complicated by the fact those casting seem unnecessary: for each of the callbacks returning bool in TAsyncChannel.h:
> 1. the returned value is never checked
> 2. they always return true
> Attached is a trivial patch based on 0.7.0, tested on Ubuntu 11.04 and Visual C++ 2010.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira