Skip to content

Avoid stale client_vc #6732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shinrich merged 2 commits into from
Jun 9, 2020
Merged

Avoid stale client_vc #6732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87bb9cc
Avoid stale client_vc
shinrich May 1, 2020
0d5be3b
Avoid write vios
shinrich May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions iocore/net/UnixNetVConnection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null read.vio cont, closing vc %p", event, vc);
Warning("read: Closing orphaned vc %p", vc);
vc->closed = 1;
break;
default:
Expand Down Expand Up @@ -119,6 +120,7 @@ write_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null write.vio cont, closing vc %p", event, vc);
Warning("write: Closing orphaned vc %p", vc);
vc->closed = 1;
break;
default:
Expand Down
24 changes: 7 additions & 17 deletions proxy/http/Http1ClientSession.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,6 @@ Http1ClientSession::free()
conn_decrease = false;
}

// Clean up the write VIO in case of inactivity timeout
this->do_io_write(nullptr, 0, nullptr);

// Free the transaction resources
this->trans.super_type::destroy();

Expand Down Expand Up @@ -228,12 +225,13 @@ Http1ClientSession::do_io_close(int alerrno)
slave_ka_vio = nullptr;
}
// Completed the last transaction. Just shutdown already
if (transact_count == released_transactions) {
// Or the do_io_close is due to a network error
if (transact_count == released_transactions || alerrno == HTTP_ERRNO) {
half_close = false;
}

// Clean up the write VIO in case of inactivity timeout
this->do_io_write(nullptr, 0, nullptr);
this->do_io_write(this, 0, nullptr);

if (half_close && this->trans.get_sm()) {
read_state = HCS_HALF_CLOSED;
Expand Down Expand Up @@ -293,11 +291,7 @@ Http1ClientSession::state_wait_for_close(int event, void *data)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
half_close = false;
Copy link
Member

@SolidWallOfCode SolidWallOfCode Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant - do_io_close will set it.

this->do_io_close();
if (_vc != nullptr) {
_vc->do_io_close();
_vc = nullptr;
}
this->do_io_close(EHTTP_ERROR);
break;
case VC_EVENT_READ_READY:
// Drain any data read
Expand Down Expand Up @@ -373,11 +367,7 @@ Http1ClientSession::state_keep_alive(int event, void *data)
break;

case VC_EVENT_EOS:
this->do_io_close();
if (_vc != nullptr) {
_vc->do_io_close();
_vc = nullptr;
}
this->do_io_close(EHTTP_ERROR);
Copy link
Member

@SolidWallOfCode SolidWallOfCode Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't EOS not be an error?

Copy link
Member

@SolidWallOfCode SolidWallOfCode Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained in person - this is to force full close, never half close. Might be better to not pass an error and explicitly set half_close = false; but it's a minor point.

break;

case VC_EVENT_READ_COMPLETE:
Expand All @@ -389,7 +379,7 @@ Http1ClientSession::state_keep_alive(int event, void *data)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
// Keep-alive timed out
this->do_io_close();
this->do_io_close(EHTTP_ERROR);
break;
}

Expand All @@ -403,7 +393,7 @@ Http1ClientSession::release(ProxyTransaction *trans)
ink_assert(read_state == HCS_ACTIVE_READER || read_state == HCS_INIT);

// Clean up the write VIO in case of inactivity timeout
this->do_io_write(nullptr, 0, nullptr);
this->do_io_write(this, 0, nullptr);

// Check to see there is remaining data in the
// buffer. If there is, spin up a new state
Expand Down
12 changes: 6 additions & 6 deletions proxy/http/HttpSM.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,8 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc, IOBufferReader *buffe
// this hook maybe asynchronous, we need to disable IO on
// client but set the continuation to be the state machine
// so if we get an timeout events the sm handles them
ua_entry->read_vio = client_vc->do_io_read(this, 0, buffer_reader->mbuf);
ua_entry->read_vio = client_vc->do_io_read(this, 0, buffer_reader->mbuf);
ua_entry->write_vio = client_vc->do_io_write(this, 0, nullptr);

/////////////////////////
// set up timeouts //
Expand Down Expand Up @@ -2763,7 +2764,7 @@ HttpSM::tunnel_handler_post(int event, void *data)
if (ua_entry->write_buffer) {
free_MIOBuffer(ua_entry->write_buffer);
ua_entry->write_buffer = nullptr;
ua_entry->vc->do_io_write(nullptr, 0, nullptr);
ua_entry->vc->do_io_write(this, 0, nullptr);
}
if (!p->handler_state) {
p->handler_state = HTTP_SM_POST_UA_FAIL;
Expand Down Expand Up @@ -3510,7 +3511,7 @@ HttpSM::tunnel_handler_post_ua(int event, HttpTunnelProducer *p)
// if it is active timeout case, we need to give another chance to send 408 response;
ua_txn->set_active_timeout(HRTIME_SECONDS(t_state.txn_conf->transaction_active_timeout_in));

p->vc->do_io_write(nullptr, 0, nullptr);
p->vc->do_io_write(this, 0, nullptr);
p->vc->do_io_shutdown(IO_SHUTDOWN_READ);

return 0;
Expand All @@ -3524,7 +3525,7 @@ HttpSM::tunnel_handler_post_ua(int event, HttpTunnelProducer *p)
// server and close the ua
p->handler_state = HTTP_SM_POST_UA_FAIL;
set_ua_abort(HttpTransact::ABORTED, event);
p->vc->do_io_write(nullptr, 0, nullptr);
p->vc->do_io_write(this, 0, nullptr);
p->vc->do_io_shutdown(IO_SHUTDOWN_READ);
tunnel.chain_abort_all(p);
server_session = nullptr;
Expand Down Expand Up @@ -3552,8 +3553,7 @@ HttpSM::tunnel_handler_post_ua(int event, HttpTunnelProducer *p)
}
}

// Initiate another read to watch catch aborts and
// timeouts
// Initiate another read to catch aborts and timeouts.
ua_entry->vc_handler = &HttpSM::state_watch_for_client_abort;
ua_entry->read_vio = p->vc->do_io_read(this, INT64_MAX, ua_buffer_reader->mbuf);
break;
Expand Down
8 changes: 2 additions & 6 deletions proxy/http2/Http2ClientSession.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,8 @@ Http2ClientSession::do_io_close(int alerrno)

this->clear_session_active();

// Clean up the write VIO in case of inactivity timeout
this->do_io_write(nullptr, 0, nullptr);
// Point the write_vio at the session in case of inactivity timeout
this->do_io_write(this, 0, nullptr);
}

void
Expand Down Expand Up @@ -345,10 +345,6 @@ Http2ClientSession::main_event_handler(int event, void *edata)
case VC_EVENT_EOS:
this->set_dying_event(event);
this->do_io_close();
if (_vc != nullptr) {
_vc->do_io_close();
_vc = nullptr;
}
retval = 0;
break;

Expand Down
5 changes: 3 additions & 2 deletions proxy/http2/Http2Stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,8 @@ Http2Stream::signal_read_event(int event)
void
Http2Stream::signal_write_event(int event)
{
if (this->write_vio.cont == nullptr || this->write_vio.cont->mutex == nullptr || this->write_vio.op == VIO::NONE) {
if (this->write_vio.cont == nullptr || this->write_vio.cont->mutex == nullptr || this->write_vio.op == VIO::NONE ||
this->write_vio.nbytes == 0) {
return;
}

Expand All @@ -684,7 +685,7 @@ Http2Stream::signal_write_event(bool call_update)
return;
}

if (this->write_vio.get_writer()->write_avail() == 0) {
if (this->write_vio.get_writer()->write_avail() == 0 || this->write_vio.nbytes == 0) {
return;
}

Expand Down