Prevent exceptions from unsubscribe_event leaving DeviceProxy destructor #521
Conversation
cppapi/client/devapi_base.cpp
Outdated
| { | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The only improvement I could think of is to take the opportunity of this Pull Request to remove this empty line.
@mliszcz, could you do that, please?
There was a problem hiding this comment.
Thanks for reviewing the PR!
@bourtemb I've removed the redundant line.
@t-b I've removed the empty() check. I've also did a few cosmetic changes and renames. I don't know what is the preferred coding style, so please check those as well.
As for the error message in catch(...), I've added a printout. Is there a logger for client API that can print at least the file and line number?
BTW In my opinion a library should not print anything to std out/err, but in case of d-tor failure there is no other way to notify the user.
There was a problem hiding this comment.
@mliszcz Looks better! Regarding the printout I would print to cerr as that is what Except::print_exception does as well, IIRC this is a log stream and not the same as std::cerr.
There was a problem hiding this comment.
@t-b thanks for checking! I've already looked into print_exception and my assumption was that cerr is std::cerr. cerr would be very unfortunate name for a custom logger stream, especially that tango.h includes iostream and brings std into default scope.
Following grep does not yield any meaningful result: grep -r cerr | grep -v 'cerr <<'
Seems that cerr actually IS std::cerr. Would it be ok for you if I'll leave the namespace in the name as it is now?
There was a problem hiding this comment.
@t-b, thanks for the review and the good catch for the empty() if.
cout (not cerr) is redirected to the log stream, except if the library is compiled in Debug mode on Linux (_TANGO_LIB compilation flag is not set in this case). In Debug mode on Linux, cout is the standard std::cout.
There is the following code in cppapi/server/logging.h:
#ifdef _TANGO_LIB //== compiling TANGO lib ====================
//-- Overwrite std::cout -------------------------------------
#define cout \
if (API_LOGGER) \
API_LOGGER->get_stream(log4tango::Level::INFO, false) \
<< log4tango::LogInitiator::_begin_log
#endif //== compiling TANGO lib ===============================
It looks like _TANGO_LIB is always defined when compiling the Tango library on Windows... So it seems cout is always redirected to the log stream on Windows, even when the library is compiled in Debug mode.
cerr is actually always std:cerr with the current source code.
As for the error message in
catch(...), I've added a printout. Is there a logger for client API that can print at least the file and line number?
Not yet.
Would it be ok for you if I'll leave the namespace in the name as it is now?
With the current code, I don't see any problem with that since cerr is std::cerr.
The question is: do we want to use a trick at some point to overload cerr with something else in a similar way as it was done for cout?
I would say if it's the case in the future, we can always modify this part of the code if needed...
We could also use cout3 to log a message to the logging system.
There was a problem hiding this comment.
@bourtemb Thanks for the insight. I'm fine with keeping it as is. We should only very rarely reach catch(...).
mliszcz
force-pushed
the
unsubscribe-event-failure-in-device-proxy-dtor
branch
from
11df11e to
c59e50d
Compare
cppapi/client/devapi_base.cpp
Outdated
| { | ||
| std::cerr | ||
| << "Unknown exception thrown from unsubscribe_event() event_id=" | ||
| << *event_id << std::endl; |
There was a problem hiding this comment.
I think we should also add the name of the device in the error message as well as the current method name.
So I would suggest to change the error message with something like:
std::cerr << " DeviceProxy::unsubscribe_all_events(): Unknown exception thrown from unsubscribe_event() for device \""
<< name() << "\" and event_id=" << *event_id << " std::endl;
There was a problem hiding this comment.
Thanks! I've updated the code according to your suggestion.
mliszcz
force-pushed
the
unsubscribe-event-failure-in-device-proxy-dtor
branch
2 times, most recently
from
efc70df to
7bd96c1
Compare
cppapi/client/devapi_base.cpp
Outdated
| { | ||
| std::cerr << | ||
| "DeviceProxy::unsubscribe_all_events(): " | ||
| "Unknown exception thrown from unsubscribe_event() for" |
There was a problem hiding this comment.
a space is missing at the end between "for" and "device"
mliszcz
force-pushed
the
unsubscribe-event-failure-in-device-proxy-dtor
branch
from
7bd96c1 to
c5c4793
Compare
* Added CHANGELOG.md This file is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). List changes since cppTango 9.2.5. Changes with potential compatibility issues are documented as suggested in #509 and are in bold. * Add links to issues and PRs in CHANGELOG.md * Update CHANGELOG.md with references to #508,#516,#518,#520,#521,#522,#523,#529,#531,#533,#537 * Update CHANGELOG.md with #517 * Update CHANGELOG.md. Add link for issue 395 * Moved 2 items from Fixed to Changed section Added a warning about the event subscription change sleep reduction * First version of the release notes for cppTango 9.3.3 Rename RELEASE_NOTES in RELEASE_NOTES.md. The previous RELEASE_NOTES content has been moved to CHANGELOG.md file. * Added reference to cppTango#539 Preparation for cppTango 9.3.3 release * Update CHANGELOG and RELEASE_NOTES with #541 and #542 * Reword one item in CHANGELOG.md and RELEASE_NOTES.md * Update CHANGELOG.md with #544, #546 and #549 * Add reference to #532 Add 9.3.3 release date * Add 9.3.3 release date
Per @bourtemb suggestion from #514 let's handle
unsubscribe_eventfailures in~DeviceProxyto prevent exceptions being thrown out of a destructor.