You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by sh...@apache.org on 2016/05/26 22:18:28 UTC
[trafficserver] branch master updated: TS-4469: TS-3612
restructuring issues causing crashes in plugins. This closes #657.
This is an automated email from the ASF dual-hosted git repository.
shinrich pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 7013e90 TS-4469: TS-3612 restructuring issues causing crashes in plugins. This closes #657.
7013e90 is described below
commit 7013e90bf20a72a78a3227cb2653b69e30d4de73
Author: Susan Hinrichs <sh...@ieee.org>
AuthorDate: Mon May 23 14:33:07 2016 +0000
TS-4469: TS-3612 restructuring issues causing crashes in plugins. This closes #657.
---
proxy/http/HttpServerSession.cc | 4 ++
proxy/http/HttpTunnel.cc | 7 +-
proxy/http2/Http2ClientSession.h | 8 ++-
proxy/http2/Http2Stream.cc | 144 ++++++++++++++++++++++++++++-----------
proxy/http2/Http2Stream.h | 9 ++-
5 files changed, 128 insertions(+), 44 deletions(-)
diff --git a/proxy/http/HttpServerSession.cc b/proxy/http/HttpServerSession.cc
index 8d0bd09..9336de3 100644
--- a/proxy/http/HttpServerSession.cc
+++ b/proxy/http/HttpServerSession.cc
@@ -180,6 +180,10 @@ HttpServerSession::release()
return;
}
+ // Make sure the vios for the current SM are cleared
+ server_vc->do_io_read(NULL, 0, NULL);
+ server_vc->do_io_write(NULL, 0, NULL);
+
HSMresult_t r = httpSessionManager.release_session(this);
if (r == HSM_RETRY) {
diff --git a/proxy/http/HttpTunnel.cc b/proxy/http/HttpTunnel.cc
index 11be120..254b05c 100644
--- a/proxy/http/HttpTunnel.cc
+++ b/proxy/http/HttpTunnel.cc
@@ -1311,7 +1311,9 @@ HttpTunnel::consumer_reenable(HttpTunnelConsumer *c)
if (is_debug_tag_set("http_tunnel"))
Debug("http_tunnel", "Unthrottle %p %" PRId64 " / %" PRId64, p, backlog, p->backlog());
srcp->unthrottle();
- srcp->read_vio->reenable();
+ if (srcp->read_vio) {
+ srcp->read_vio->reenable();
+ }
// Kick source producer to get flow ... well, flowing.
this->producer_handler(VC_EVENT_READ_READY, srcp);
} else {
@@ -1325,7 +1327,8 @@ HttpTunnel::consumer_reenable(HttpTunnelConsumer *c)
}
}
}
- p->read_vio->reenable();
+ if (p->read_vio)
+ p->read_vio->reenable();
}
}
}
diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h
index bfb156f..d24ee98 100644
--- a/proxy/http2/Http2ClientSession.h
+++ b/proxy/http2/Http2ClientSession.h
@@ -180,7 +180,13 @@ public:
virtual void
release_netvc()
{
- client_vc = NULL;
+ // Make sure the vio's are also released to avoid
+ // later surprises in inactivity timeout
+ if (client_vc) {
+ client_vc->do_io_read(NULL, 0, NULL);
+ client_vc->do_io_write(NULL, 0, NULL);
+ client_vc = NULL;
+ }
}
sockaddr const *
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index 42f6659..72c021b 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -36,9 +36,7 @@ Http2Stream::main_event_handler(int event, void *edata)
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
if (e == cross_thread_event) {
cross_thread_event = NULL;
- }
-
- if (e == active_event) {
+ } else if (e == active_event) {
event = VC_EVENT_ACTIVE_TIMEOUT;
active_event = NULL;
} else if (e == inactive_event) {
@@ -46,6 +44,10 @@ Http2Stream::main_event_handler(int event, void *edata)
event = VC_EVENT_INACTIVITY_TIMEOUT;
clear_inactive_timer();
}
+ } else if (e == read_event) {
+ read_event = NULL;
+ } else if (e == write_event) {
+ write_event = NULL;
}
switch (event) {
case VC_EVENT_ACTIVE_TIMEOUT:
@@ -212,7 +214,7 @@ Http2Stream::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf)
// Is there already data in the request_buffer? If so, copy it over and then
// schedule a READ_READY or READ_COMPLETE event after we return.
- update_read_request(nbytes, true);
+ update_read_request(nbytes, false);
return &read_vio;
}
@@ -239,19 +241,27 @@ Http2Stream::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *abuffe
void
Http2Stream::do_io_close(int /* flags */)
{
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
current_reader = NULL; // SM on the way out
if (!sent_delete) {
+ sent_delete = true;
Debug("http2_stream", "do_io_close stream %d", this->get_id());
// Only close if we are done sending data back to the client
if (parent && (!this->is_body_done() || this->response_is_data_available())) {
Debug("http2_stream", "%d: Undo close to pass data", this->get_id());
- closed = false; // "unclose" so this gets picked up later when the netvc side is done
- this->reenable(&write_vio); // Kick the mechanism to get any remaining data pushed out
- return;
+ closed = false; // "unclose" so this gets picked up later when the netvc side is done
+ // If chunking is playing games with us, make sure we noticed when the end of message has happened
+ if (!this->is_body_done() && this->write_vio.ndone == this->write_vio.nbytes) {
+ this->mark_body_done();
+ } else {
+ lock.release();
+ this->reenable(&write_vio); // Kick the mechanism to get any remaining data pushed out
+ Warning("Re-enabled to get data pushed out is_done=%d", this->is_body_done());
+ return;
+ }
}
closed = true;
- sent_delete = true;
if (parent) {
// Make sure any trailing end of stream frames are sent
@@ -262,9 +272,8 @@ Http2Stream::do_io_close(int /* flags */)
}
parent = NULL;
- SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
-
clear_timers();
+ clear_io_events();
if (cross_thread_event != NULL)
cross_thread_event->cancel();
@@ -291,6 +300,7 @@ Http2Stream::initiating_close()
parent = NULL;
clear_timers();
+ clear_io_events();
// This should result in do_io_close or release being called. That will schedule the final
// kill yourself signal
@@ -333,8 +343,25 @@ Http2Stream::initiating_close()
}
}
+/* Replace existing event only if the new event is different than the inprogress event */
+Event *
+Http2Stream::send_tracked_event(Event *in_event, int send_event, VIO *vio)
+{
+ Event *event = in_event;
+ if (event != NULL) {
+ if (event->callback_event != send_event) {
+ event->cancel();
+ event = NULL;
+ }
+ }
+ if (event == NULL) {
+ event = this_ethread()->schedule_imm(this, send_event, vio);
+ }
+ return event;
+}
+
void
-Http2Stream::update_read_request(int64_t read_len, bool send_update)
+Http2Stream::update_read_request(int64_t read_len, bool call_update)
{
if (closed || this->current_reader == NULL)
return;
@@ -347,33 +374,43 @@ Http2Stream::update_read_request(int64_t read_len, bool send_update)
return;
}
ink_release_assert(this->get_thread() == this_ethread());
- if (send_update) {
- SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread());
- if (read_vio.nbytes > 0 && read_vio.ndone <= read_vio.nbytes) {
- // If this vio has a different buffer, we must copy
- ink_release_assert(this_ethread() == this->_thread);
- if (read_vio.buffer.writer() != (&request_buffer)) {
- int64_t num_to_read = read_vio.nbytes - read_vio.ndone;
- if (num_to_read > read_len)
- num_to_read = read_len;
- if (num_to_read > 0) {
- int bytes_added = read_vio.buffer.writer()->write(request_reader, num_to_read);
- if (bytes_added > 0) {
- request_reader->consume(bytes_added);
- read_vio.ndone += bytes_added;
- int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE : VC_EVENT_READ_READY;
- this_ethread()->schedule_imm(this, send_event, &read_vio);
- // this->handleEvent(send_event, &read_vio);
+ SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread());
+ if (read_vio.nbytes > 0 && read_vio.ndone <= read_vio.nbytes) {
+ // If this vio has a different buffer, we must copy
+ ink_release_assert(this_ethread() == this->_thread);
+ if (read_vio.buffer.writer() != (&request_buffer)) {
+ int64_t num_to_read = read_vio.nbytes - read_vio.ndone;
+ if (num_to_read > read_len)
+ num_to_read = read_len;
+ if (num_to_read > 0) {
+ int bytes_added = read_vio.buffer.writer()->write(request_reader, num_to_read);
+ if (bytes_added > 0) {
+ request_reader->consume(bytes_added);
+ read_vio.ndone += bytes_added;
+ int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE : VC_EVENT_READ_READY;
+ // If call_update is true, should be safe to call the read_io continuation handler directly
+ // However, I was seeing performance regressions, so backed out this change to track that down
+ // Probably not the cause of performance regression, but need to test some more
+ /*if (call_update) { // Safe to call vio handler directly
+ inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+ if (read_vio._cont && this->current_reader) read_vio._cont->handleEvent(send_event, &read_vio);
+ } else */ { // Called from do_io_read. Still setting things up. Send
+ // event to handle this after the dust settles
+ read_event = send_tracked_event(read_event, send_event, &read_vio);
}
- ink_release_assert(!this->closed);
}
- } else {
- // Try to be smart and only signal if there was additional data
- if (request_reader->read_avail() > 0) {
- int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE : VC_EVENT_READ_READY;
- this_ethread()->schedule_imm(this, send_event, &read_vio);
- // this->handleEvent(send_event, &read_vio);
- ink_release_assert(!this->closed);
+ }
+ } else {
+ // Try to be smart and only signal if there was additional data
+ int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE : VC_EVENT_READ_READY;
+ if (request_reader->read_avail() > 0 || send_event == VC_EVENT_READ_COMPLETE) {
+ // Same comment of call_update as above
+ /*if (call_update) { // Safe to call vio handler directly
+ inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+ if (read_vio._cont && this->current_reader) read_vio._cont->handleEvent(send_event, &read_vio);
+ } else */ { // Called from do_io_read. Still setting things up. Send event
+ // to handle this after the dust settles
+ read_event = send_tracked_event(read_event, send_event, &read_vio);
}
}
}
@@ -381,7 +418,7 @@ Http2Stream::update_read_request(int64_t read_len, bool send_update)
}
bool
-Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len, bool send_update)
+Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len, bool call_update)
{
bool retval = true;
if (closed || parent == NULL)
@@ -438,7 +475,16 @@ Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len,
// make sure to send the end of stream
if (this->response_is_data_available() || send_event == VC_EVENT_WRITE_COMPLETE) {
if (send_event != VC_EVENT_WRITE_COMPLETE) {
- this_ethread()->schedule_imm(this, VC_EVENT_WRITE_READY, &write_vio);
+ // As with update_read_request, should be safe to call handler directly here if
+ // call_update is true. Commented out for now while tracking a performance regression
+ /*if (call_update) { // Coming from reenable. Safe to call the handler directly
+ inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+ if (write_vio._cont && this->current_reader) write_vio._cont->handleEvent(send_event, &write_vio);
+ } else */ { // Called from do_io_write. Might
+ // still be setting up state. Send
+ // an event to let the dust settle
+ write_event = send_tracked_event(write_event, send_event, &write_vio);
+ }
} else {
this->mark_body_done();
retval = false;
@@ -462,9 +508,16 @@ Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len,
send_response_body();
retval = false;
} else {
- this_ethread()->schedule_imm(this, VC_EVENT_WRITE_READY, &write_vio);
send_response_body();
- // write_vio._cont->handleEvent(send_event, &write_vio);
+ // Same comment about call_update as above
+ /*if (call_update) { // Coming from reenable. Safe to call the handler directly
+ inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+ if (write_vio._cont && this->current_reader) write_vio._cont->handleEvent(send_event, &write_vio);
+ } else */ { // Called from do_io_write. Might still
+ // be setting up state. Send an event to
+ // let the dust settle
+ write_event = send_tracked_event(write_event, send_event, &write_vio);
+ }
}
}
@@ -657,3 +710,14 @@ Http2Stream::clear_timers()
clear_inactive_timer();
clear_active_timer();
}
+
+void
+Http2Stream::clear_io_events()
+{
+ if (read_event)
+ read_event->cancel();
+ read_event = NULL;
+ if (write_event)
+ write_event->cancel();
+ write_event = NULL;
+}
diff --git a/proxy/http2/Http2Stream.h b/proxy/http2/Http2Stream.h
index 51606f0..1008072 100644
--- a/proxy/http2/Http2Stream.h
+++ b/proxy/http2/Http2Stream.h
@@ -61,7 +61,9 @@ public:
chunked(false),
cross_thread_event(NULL),
active_event(NULL),
- inactive_event(NULL)
+ inactive_event(NULL),
+ read_event(NULL),
+ write_event(NULL)
{
SET_HANDLER(&Http2Stream::main_event_handler);
}
@@ -226,8 +228,10 @@ public:
void clear_inactive_timer();
void clear_active_timer();
void clear_timers();
+ void clear_io_events();
private:
+ Event *send_tracked_event(Event *event, int send_event, VIO *vio);
HTTPParser http_parser;
ink_hrtime _start_time;
EThread *_thread;
@@ -255,6 +259,9 @@ private:
ink_hrtime inactive_timeout;
ink_hrtime inactive_timeout_at;
Event *inactive_event;
+
+ Event *read_event;
+ Event *write_event;
};
extern ClassAllocator<Http2Stream> http2StreamAllocator;
--
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].