上一篇寫到 不識貨的我不欣賞 ActiveMQ-CPP "仿 Java" 的 design 哲學. 或許是我太機車了, 畢竟這是主觀的. 一個特定 design 哲學及其產物一定是有好也有壞, 評價的高低端看它的強項是否能被人欣賞, 缺點是否能被接受 (忍受). 今天讓我們來幫 ActiveMQ-CPP 做個 code review, 從 C++ coding 來看看能不能幫 Apache 扳回一城.

先從 ActiveMQ-CPP 的一個 example 看起. 下面是其中一個 class 的 dtor (destructor) 的 code snip (details omitted):

Listing 1
PLAIN TEXT
C++:
1. class HelloWorldProducer : public Runnable {
2. public:
3.     virtual ~HelloWorldProducer(){
4.         cleanup();
5.     }
6.  
7. private:
8.     void cleanup(){
9.  
10.         // Destroy resources.
11.         try{
12.             if( destination != NULL ) delete destination;
13.         }catch ( CMSException& e ) { e.printStackTrace(); }
14.         destination = NULL;
15.  
16.         try{
17.             if( producer != NULL ) delete producer;
18.         }catch ( CMSException& e ) { e.printStackTrace(); }
19.         producer = NULL;
20.  
21.         // Close open resources.
22.         try{
23.             if( session != NULL ) session->close();
24.             if( connection != NULL ) connection->close();
25.         }catch ( CMSException& e ) { e.printStackTrace(); }
26.  
27.         try{
28.             if( session != NULL ) delete session;
29.         }catch ( CMSException& e ) { e.printStackTrace(); }
30.         session = NULL;
31.  
32.         try{
33.             if( connection != NULL ) delete connection;
34.         }catch ( CMSException& e ) { e.printStackTrace(); }
35.         connection = NULL;
36.     }
37. };

Listing 1 的 line 12, 17, 23, 28, 33 都在 delete 之前先測試要被 delete 的 pointer 是否為 NULL. 這其實是 完全沒有必要的. 至少在使用符合 Standard 規範的 Standard Library 實作或是符合 Standard 規範的 user-defined operator delete 時沒有必要.

接著可以注意到這些 delete statement 都位於 try block 裏面. 這比較奇怪, Apache 認可的 C++ contributor 還有人不知道當 object 在 destroy 的時候遇到錯誤, 寫個 log 甚至是打電話給你的阿姨都比在 dtor 裏面拋出 exception 來得好!¹ 在這驚魂未定之際 trace 了幾個被 delete 的 class (from derived to base classes) 的 dtor, 發現它們都是空的, 也就是說根本不會有 exception 發生的可能. 更別提不允許 dtor 拋出 exception 是 Standard 明白規定的.

也許 ActiveMQ-CPP 的人不認同 Standard 或是其他 Guru 的意見, 寧願讓 dtor 可以拋出 exception. 而為了搞不清楚到底哪些會 throw, 就全部 try/catch 起來. 在我沒看完所有的 dtor code 的狀況下, 我願意相信 Apache/ActiveMQ 的人不是出於對 C++ 的熟悉度不夠, 而是選擇這樣做的. 其實這個猜測已經與 HelloWorldProducer 的 dtor 的思維背道而馳了, One allows exceptions to propagate out of dtors, the other catches and absorbs all exceptions in dtor. 撇開這些的有的沒的, HelloWorldProducer 的 dtor 已是萬無一失了嗎? Well, not quite.

HelloWorldProducer::cleanup() 的所有 catch block 裏面都做了同樣一件事 - 把 exception 所經過的 stack trace 輸出到 std::cerr, i.e. e.printStackTrace(). Code snip from decaf/lang/Exception.cpp:

Listing 2
PLAIN TEXT
C++:
1. void Exception::printStackTrace() const {
2.     printStackTrace( std::cerr );
3. }
4.  
5. ////////////////////////////////////////////////////////////////////////////////
6. void Exception::printStackTrace( std::ostream& stream ) const {
7.     stream <<getStackTraceString();
8. }
9.  
10. ////////////////////////////////////////////////////////////////////////////////
11. std::string Exception::getStackTraceString() const {
12.  
13.     // Create the output stream.
14.     std::ostringstream stream;
15.  
16.     // Write the message and each stack entry.
17.     stream <<message <<std::endl;
18.     for( unsigned int ix=0; ix<stackTrace.size(); ++ix ){
19.         stream <<"\tFILE: " <<stackTrace[ix].first;
20.         stream <<", LINE: " <<stackTrace[ix].second;
21.         stream <<std::endl;
22.     }
23.  
24.     // Return the string from the output stream.
25.     return stream.str();
26. }

Line 2 and 7, 在沒有檢查 exception mask 的前提下, 對 std::cerr 做 insertion 有可能拋出 exception. 雖然 default 是不會, i.e. basic_ios::exceptions() 為 ios_base::goodbit, 但 std::cerr 為一 global variable, 誰能保證它的 exception mask 不會被 client code (甚至是 ActiveMQ-CPP 的其他部份) 改變?

說到 IOStream default 不會 throw, 因此在 Exception::getStackTraceString() 內對一個 local 的 std::ostringstream instance (stream) 做 insertion 不會 throw. 但是 stream.str() (basic_ostringstream::str()) - 由於以 return by value 的方式回傳一個 std::string - 卻有可能 throw.² 結果就是 HelloWorldProducer 的 dtor 不但沒有成功地將所有可能的 exception 攔住, 反而增加了在 catch block 內發生沒被處理的 exception 的可能. 而比在 dtor 內拋出 exception 更糟糕的是 - 這狀況會讓程式立即 terminate.

ActiveMQ 是實作來給 enterprise grade application 使用, 做為做 message transport/routing, 該要 "非常非常 robust" 才對. 上面談到的問題點畢竟都不是源自 production code, 或許不該被拿來太認真檢驗, 只能說它是很糟糕的 sample code. 可是往更深入的地方繼續 hack, 看到的卻是整個 ActiveMQ-CPP 的 exception handling 都不及格.

隨便選個檔案挑個 function, activemq/core/ActiveMQConnection.cpp:

Listing 3
PLAIN TEXT
C++:
1. ActiveMQConnection::~ActiveMQConnection() {
2.     try {
3.         this->close();
4.     }
5.     AMQ_CATCH_NOTHROW( ActiveMQException )
6.     AMQ_CATCHALL_NOTHROW( )
7. }

Note: 挑 ActiveMQConnection 的 dtor 來說明是因為它短, 與它是個 dtor 無關.

其中 AMQ_CATCH_NOTHROW 的定義如下 (in activemq/exceptions/ExceptionDefines.h):

Listing 4
PLAIN TEXT
C++:
1. #define AMQ_CATCH_NOTHROW( type ) \
2.     catch( type& ex ){ \
3.         ex.setMark( __FILE__, __LINE__ ); \
4.     }

而 ActiveMQConnection::setMark() (source) 為一 proxy function, 轉 call 其 base class - decaf::lang::Exception (source) 的 setMark():

Listing 5
PLAIN TEXT
C++:
1. void Exception::setMark( const char* file, const int lineNumber ) {
2.  
3.     // Add this mark to the end of the stack trace.
4.     stackTrace.push_back( std::make_pair( (std::string)file, (int)lineNumber ) );
5.  
6.     std::ostringstream stream;
7.     stream <<"\tFILE: " <<stackTrace[stackTrace.size()-1].first;
8.     stream <<", LINE: " <<stackTrace[stackTrace.size()-1].second;
9.  
10.     //decaf::util::logger::SimpleLogger logger("com.yadda2");
11.     //logger.log( stream.str() );
12. }

不讓人意外, 光看 listing 5 的第四行就可以猜到 Exception::stackTrace 的型別應該是 std::vector< std::pair< std::string, int> >. 而單是這一行 code, 就 (至少) 有四個地方會產生 exception. (你知道是哪四個嗎?) 看到這裡, 相信你已經知道這個 function 在 catch block 內被呼叫, 若引發了沒有被處理的 exception 的後果 - terminate. 而這樣的 code, 在 ActiveMQ-CPP 裏面到處都是.

ActiveMQ 本身 (由於是以 Java 寫的) 是否不受這篇談的 exception handling 缺失的影響? 雖然 Java 我是門外漢, 但就我所知 C++ 與 Java 的 exception handling model 是很接近的. 因此, 若 hack 進去可能也會發現類似的問題. 那 ActiveMQ-CPP 能用嗎? 除了前面提到的問題之外, 另外還有注意到模組化, 同步最佳化等的 non-critical 問題, 或許我會另外寫一篇文字加以討論.³ 對 JMS/ActiveMQ 不熟悉的我, 要用看的抓出 ActiveMQ-CPP 非語言問題相關的其他問題可說是力有未逮 (at least at the time of writing). 無論如何, 一個打著 Apache 招牌的 project, 我相信它起碼在理想狀態下是能正確運作的, 至少大多數時候.

由於工作上需要使用 ActiveMQ/ActiveMQ-CPP, 希望能在這一兩個星期之內準備個 patch (maybe a few patches) 送給 ActiveMQ-CPP. Java (ActiveMQ) 的部份就要靠另外的善心人士幫幫忙看看有沒有問題.

1. From [17.3] of C++ Faq Lite [↩]
2. Exception::getStackTraceString() 也是 return by value, 因此也一樣可能 throw. [↩]
3. 其實 Listing 5 就有無用的廢碼. [↩]