Add logic to clean up vios on HttpSM shutdown #4020
Conversation
proxy/http/Http1ClientSession.cc
Outdated
| Http1ClientSession::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) | ||
| { | ||
| return client_vc->do_io_read(c, nbytes, buf); | ||
| if (client_vc) { |
There was a problem hiding this comment.
return client_vc ? client_vc->do_io_read(c, nbytes, buf) : nullptr; ?
proxy/http/HttpSM.cc
Outdated
| // | ||
| void | ||
| HttpVCTable::remove_entry(HttpVCTableEntry *e) | ||
| HttpVCTable::remove_entry(HttpVCTableEntry *e, HttpSM *sm) |
There was a problem hiding this comment.
If this is needed for various methods, perhaps HttpVCTable should be constructed with a reference to the HttpSM.
proxy/http/HttpSM.cc
Outdated
| if (e->read_vio != nullptr && e->read_vio->cont == sm) { | ||
| // Cleanup dangling i/o | ||
| if (e == sm->get_ua_entry() && sm->get_ua_txn()) { | ||
| e->read_vio = sm->get_ua_txn()->do_io_read(NULL, 0, NULL); |
There was a problem hiding this comment.
The read_vio will get cleaned out later, so this doesn't repeatedly shut down IO?
proxy/http/HttpSM.cc
Outdated
| } | ||
| if (e->read_vio != nullptr && e->read_vio->cont == sm) { | ||
| // Cleanup dangling i/o | ||
| if (e == sm->get_ua_entry() && sm->get_ua_txn()) { |
There was a problem hiding this comment.
style consistency - if you check vs. nullptr in the previous if, do that here too with sm->get_ua_txn().
| break; | ||
| case VC_EVENT_ERROR: | ||
| case VC_EVENT_INACTIVITY_TIMEOUT: | ||
| case VC_EVENT_ACTIVE_TIMEOUT: |
There was a problem hiding this comment.
Where was VC_EVENT_ACTIVE_TIMEOUT handled before? Or was it not handled?
There was a problem hiding this comment.
The active timeout was not handled before. Ran across it while debugging the ASYNC job. Should be handled here. I can pull it into a separate PR if necessary.
proxy/http/HttpSM.cc
Outdated
| ink_assert(p->vc == server_session); | ||
|
|
||
| // The server session has been released. Clean all pointer | ||
| server_entry->in_tunnel = true; // to avid cleaning in clenup_entry |
There was a problem hiding this comment.
This seems an odd place to set in_tunnel to true.
| ua_txn->set_active_timeout(HRTIME_SECONDS(t_state.txn_conf->transaction_active_timeout_in)); | ||
|
|
||
| p->vc->do_io_write(this, 0, nullptr); | ||
| p->vc->do_io_write(nullptr, 0, nullptr); |
There was a problem hiding this comment.
Ah, that's how you stop repeat shutdowns.
proxy/http/HttpServerSession.cc
Outdated
| HttpServerSession::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) | ||
| { | ||
| return server_vc->do_io_read(c, nbytes, buf); | ||
| if (server_vc) { |
There was a problem hiding this comment.
I suppose it's just a style thing, but I prefer the single return with ternary style.
|
Pushed a new version which should address Alan's concerns. |
| HttpVCTable::HttpVCTable(HttpSM *mysm) | ||
| { | ||
| memset(&vc_table, 0, sizeof(vc_table)); | ||
| sm = mysm; |
There was a problem hiding this comment.
Use member initializer. E.g.
HttpVCTable::HttpVCTable(HttpSM *state_machine) : sm(state_machine)
| Http1ClientSession::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) | ||
| { | ||
| return client_vc->do_io_read(c, nbytes, buf); | ||
| return (client_vc) ? client_vc->do_io_read(c, nbytes, buf) : nullptr; |
There was a problem hiding this comment.
Why the parentheses around client_vc?
|
I'm going to remove the 7.1.5 Project on this, since going forward, we will need to make a second PR against the 7.1.x for proposed back ports. |
|
Cherry picked to 8.1.0 |
4 participants
Added this logic while debugging an openssl engine using ASYNC jobs. This and PR #4019 are underlying issues I fixed that were revealed by the timing differences of the engine.
This PR cleans up any VIOs against the the HttpSM that is being killed. This ends up preventing inactivity timeouts on stale HttpSM's.