Fixed memory leaking introduced by 4873

[scw00]

Based on #4873, The ua_txn set to nullptr, transaction_done is never called by HttpSM. So the Session is never freed.

[scw00]

link to #5042

[oknet]

As the state machine implementer,

• once the state machine has finished using this VConnection, the state machine must call do_io_close to indicate that the VConnection can be deallocated.
• the state machine must not access the VConnection or any VIOs obtained from it after calling this method.

As the underlying processor implementer,

• after a close has been called, the underlying processor must not send any more events related to this VConnection to the state machine.

Setting the ua_txn to nullptr is right thing. We should review the design of transaction_done if this makes any mistake.

The comments in I_VConnection.h for VConnection::do_io_close:

256   /**
257     Indicate that the VConnection is no longer needed.
258 
259     Once the state machine has finished using this VConnection, it
260     must call this function to indicate that the VConnection can
261     be deallocated.  After a close has been called, the VConnection
262     and underlying processor must not send any more events related
263     to this VConnection to the state machine. Likeswise, the state
264     machine must not access the VConnection or any VIOs obtained
265     from it after calling this method.
266 
267     @param lerrno indicates where a close is a normal close or an
268       abort. The difference between a normal close and an abort
269       depends on the underlying type of the VConnection.
270 
271   */
272   virtual void do_io_close(int lerrno = -1) = 0;

[SolidWallOfCode]

I think I agree with @oknet - the call to do_io_close should suffice to clean up. If not, we need to look at that. Having a dangling VConn that's been closed is unlikely to end well.

[shinrich]

We call transcation_done from the HttpSM::kill_this() to allow for the TXN_CLOSE hook to be called at the proper end of the transaction. As I recall, trying to trigger the TXN_CLOSE hooks from the ProxyTransaction::do_io_close() was problematic. There were cases when the transaction wasn't quite done.

The ua_txn->do_io_close() will shutdown the VConnection associated with the user agent and null out the reference, so there is no dangling VConnection. However, we still need the ProxyTransaction object (ua_txn), so we can signal TXN_CLOSE when the HttpSM is really shut down.

In my opinion, the proposed fix is good.

[duke8253]

Nice, was planning on looking at this issue.

[masaori335]

For workaround fix, I'm fine with this. But I agree with oknet, we should revisit design of ProxyTransaction and VConnection.

[scw00]

Let’s open an issue to trace this case.

[scw00]

link to #5052

[SolidWallOfCode]

@shinrich talked to me about this, and convinced me this is the correct approach. The key point was the ua_txn object serves as an intermediary between the HttpSM and the NetVC. It is expected to handle doing the NetVC cleanup, the HttpSM shouldn't get involved. The transaction object will handle not touching the NetVC again, the HttpSM should just trust it to do the right thing. The point here, by @scw00, is that it is this transaction object that is getting leaked, not the NetVC.

[scw00]

Yes, I introduced remove_entry because the NetVC will be destroyed after we return back to NetHandler. And the HttpSM::kill_this should not access vio anymore (in HttpSM::kill_this ----->vc_table.cleanup_all) .

Is it possible to do ProxyTransaction as a VConnection ? As the comment said "* the state machine must not access the VConnection or any VIOs obtained from it after calling this method."

class HttpSM;
class HttpServerSession;
class ProxyClientTransaction : public VConnection
{
public:
  ProxyClientTransaction();

  // do_io methods implemented by subclasses

  virtual void new_transaction();

  virtual NetVConnection *
  get_netvc() const
  {
    return (parent) ? parent->get_netvc() : nullptr;
  }

[scw00]

I will merge this PR if there is no objection.

[shinrich]

+1 on the merge.

In terms of the concern about the semantics of ::do_io_close on the ua_txn object. Would it be clearer and more consistent if we named that function ::release instead so there was no expectation that the ua_txn object was inaccessible after the call? So

ua_txn->release() (which would still call do_io_close on the underlying netvc)

[scw00]

IIUC, class ProxyClientTransaction : public VConnection means we can pass ProxyClientTransaction as VConnection directly (we don't use it in current logic).

When some one want to call VConnection::do_io_close it doesn't call transaction_done because there is no transaction_done in VConnection.

IIUC, do_io_close means close txn directly that means the session is terminated and it will not create a new transaction any more and will be destroyed after (or before) client's connection closed.

release means keep alive. That Session is waiting for next request and create a new transaction for that request.

I think these are different semantics.