[GitHub] [trafficserver] maskit commented on pull request #7258: Fix proxy.process.http.current_client_transactions

[GitBox <gi...@apache.org>]

maskit commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-705903123


   I think adding another line that calls `decrement_client_transactions_stat` is not a good idea. Having multiple paths would make things complicated.
   
   `ProxyTransaction::decrement_client_transactions_stat` is only called from `ProxyTransaction::release`.
   `Http1ClientSession::release_transaction()`, which is the one you added the line, is only called from `Http1Transaction::transaction_done`.
   
   If adding the line into `release_transaction` solves the issue, doesn't it mean `ProxyTransaction::release` is not called for the transaction? I think that is the root cause.
   
   This code looks suspicious. When subclass's `release` is called, it should always call superclass's `release`.
   https://github.com/apache/trafficserver/blob/718bef406aacdb14564cb111c3ad14cb9bc1466d/proxy/http/Http1Transaction.cc#L42-L46
   
   Even if this works for the stat issue, a risk for not running common processes on releasing transaction would be left. Since it's a blocker for 9.0, I didn't make a change request. However, I'm not so happy with this approach.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org