Skip to content
This repository was archived by the owner on Apr 10, 2025. It is now read-only.

Commit a68006c

Browse files
committed
backport apache fetch buffering to branch 32
1 parent 8baf27d commit a68006c

File tree

9 files changed

+366
-166
lines changed

9 files changed

+366
-166
lines changed

net/instaweb/apache/apache_slurp.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ void SlurpUrl(ApacheServerContext* server_context, request_rec* r) {
270270
// We always disable downstream header filters when sending out
271271
// slurped resources, since we've captured them from the origin
272272
// in the fetch we did to write the slurp.
273-
ApacheWriter apache_writer(r);
273+
ApacheWriter apache_writer(r, server_context->thread_system());
274274
apache_writer.set_disable_downstream_header_filters(true);
275275
ChunkingWriter chunking_writer(
276276
&apache_writer, global_config->slurp_flush_limit());

net/instaweb/apache/apache_writer.cc

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,33 +27,35 @@
2727

2828
namespace net_instaweb {
2929

30-
ApacheWriter::ApacheWriter(request_rec* r)
30+
ApacheWriter::ApacheWriter(request_rec* r, ThreadSystem* thread_system)
3131
: request_(r),
3232
headers_out_(false),
3333
disable_downstream_header_filters_(false),
3434
strip_cookies_(false),
35-
squelch_output_(false),
36-
content_length_(AsyncFetch::kContentLengthUnknown) {
35+
content_length_(AsyncFetch::kContentLengthUnknown),
36+
thread_system_(thread_system) {
37+
apache_request_thread_.reset(thread_system->GetThreadId());
3738
}
3839

3940
ApacheWriter::~ApacheWriter() {
4041
}
4142

4243
bool ApacheWriter::Write(const StringPiece& str, MessageHandler* handler) {
44+
DCHECK(apache_request_thread_->IsCurrentThread());
4345
DCHECK(headers_out_);
44-
if (!squelch_output_) {
45-
ap_rwrite(str.data(), str.size(), request_);
46-
}
46+
ap_rwrite(str.data(), str.size(), request_);
4747
return true;
4848
}
4949

5050
bool ApacheWriter::Flush(MessageHandler* handler) {
51+
DCHECK(apache_request_thread_->IsCurrentThread());
5152
DCHECK(headers_out_);
5253
ap_rflush(request_);
5354
return true;
5455
}
5556

5657
void ApacheWriter::OutputHeaders(ResponseHeaders* response_headers) {
58+
DCHECK(apache_request_thread_->IsCurrentThread());
5759
DCHECK(!headers_out_);
5860
if (headers_out_) {
5961
return;

net/instaweb/apache/apache_writer.h

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
#define NET_INSTAWEB_APACHE_APACHE_WRITER_H_
1919

2020
#include "net/instaweb/util/public/basictypes.h"
21+
#include "net/instaweb/util/public/scoped_ptr.h"
2122
#include "net/instaweb/util/public/string_util.h"
23+
#include "net/instaweb/util/public/thread_system.h"
2224
#include "net/instaweb/util/public/writer.h"
2325

2426
struct request_rec;
@@ -28,10 +30,12 @@ namespace net_instaweb {
2830
class MessageHandler;
2931
class ResponseHeaders;
3032

31-
// Writer object that writes to an Apache Request stream.
33+
// Writer object that writes to an Apache Request stream. Should only be used
34+
// from a single apache request thread, not from a rewrite thread or anything
35+
// else.
3236
class ApacheWriter : public Writer {
3337
public:
34-
explicit ApacheWriter(request_rec* r);
38+
ApacheWriter(request_rec* r, ThreadSystem* thread_system);
3539
virtual ~ApacheWriter();
3640

3741
virtual bool Write(const StringPiece& str, MessageHandler* handler);
@@ -61,18 +65,14 @@ class ApacheWriter : public Writer {
6165
strip_cookies_ = x;
6266
}
6367

64-
// When proxying content we deem to be unsafe (e.g. lacking
65-
// a Content-Type header) we must squelch the output.
66-
void set_squelch_output(bool x) { squelch_output_ = true; }
67-
bool squelch_output() const { return squelch_output_; }
68-
6968
private:
7069
request_rec* request_;
7170
bool headers_out_;
7271
bool disable_downstream_header_filters_;
7372
bool strip_cookies_;
74-
bool squelch_output_;
7573
int64 content_length_;
74+
ThreadSystem* thread_system_;
75+
scoped_ptr<ThreadSystem::ThreadId> apache_request_thread_;
7676

7777
DISALLOW_COPY_AND_ASSIGN(ApacheWriter);
7878
};

net/instaweb/apache/instaweb_handler.cc

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ InstawebHandler::~InstawebHandler() {
128128
// If fetch_ is null we either never tried to fetch anything or it took
129129
// ownership of itself after timing out.
130130
if (fetch_ != NULL) {
131-
if (WaitForFetch() == ApacheFetch::kWaitSuccess) {
131+
if (WaitForFetch()) {
132132
delete fetch_; // Fetch completed normally.
133133
} else {
134134
// Fetch took ownership of itself and will continue in the background.
@@ -137,19 +137,19 @@ InstawebHandler::~InstawebHandler() {
137137
}
138138
}
139139

140-
ApacheFetch::WaitResult InstawebHandler::WaitForFetch() {
140+
bool InstawebHandler::WaitForFetch() {
141141
if (fetch_ == NULL) {
142-
return ApacheFetch::kWaitSuccess; // Nothing to wait for.
142+
return true; // Nothing to wait for.
143143
}
144144

145-
ApacheFetch::WaitResult wait_result = fetch_->Wait(rewrite_driver_);
146-
if (wait_result != ApacheFetch::kWaitSuccess) {
145+
bool ok = fetch_->Wait(rewrite_driver_);
146+
if (!ok) {
147147
// Give up on waiting for the fetch so we stop tying up a thread. Fetch has
148148
// taken ownership of itself, will discard any messages it receives if it's
149149
// abandoned, and will delete itself when Done() is called, if ever.
150150
fetch_ = NULL;
151151
}
152-
return wait_result;
152+
return ok;
153153
}
154154

155155
void InstawebHandler::SetupSpdyConnectionIfNeeded() {
@@ -179,16 +179,19 @@ RewriteDriver* InstawebHandler::MakeDriver() {
179179
}
180180

181181
ApacheFetch* InstawebHandler::MakeFetch(const GoogleString& url,
182+
bool buffered,
182183
StringPiece debug_info) {
183184
DCHECK(fetch_ == NULL);
184185
// ApacheFetch takes ownership of request_headers.
185186
RequestHeaders* request_headers = new RequestHeaders();
186187
ApacheRequestToRequestHeaders(*request_, request_headers);
187-
fetch_ = new ApacheFetch(url, debug_info, server_context_->thread_system(),
188-
server_context_->timer(),
189-
new ApacheWriter(request_),
190-
request_headers, request_context_, options_,
191-
server_context_->message_handler());
188+
fetch_ = new ApacheFetch(
189+
url, debug_info, server_context_->thread_system(),
190+
server_context_->timer(),
191+
new ApacheWriter(request_, server_context_->thread_system()),
192+
request_headers, request_context_, options_,
193+
server_context_->message_handler());
194+
fetch_->set_buffered(buffered);
192195
return fetch_;
193196
}
194197

@@ -441,13 +444,12 @@ bool InstawebHandler::HandleAsInPlace() {
441444
!= NULL) || (request_->user != NULL));
442445

443446
RewriteDriver* driver = MakeDriver();
444-
MakeFetch(original_url_, "ipro");
447+
MakeFetch(true /* buffered */, "ipro");
445448
fetch_->set_handle_error(false);
446449
driver->FetchInPlaceResource(stripped_gurl_, false /* proxy_mode */, fetch_);
447-
ApacheFetch::WaitResult wait_result = WaitForFetch();
448-
if (wait_result != ApacheFetch::kWaitSuccess) {
449-
// Note: fetch_ has been released; no longer safe to look at;
450-
handled = (wait_result == ApacheFetch::kAbandonedAndHandled);
450+
bool ok = WaitForFetch();
451+
if (!ok) {
452+
// Nothing to do, fetch_ has been released, no longer safe to look at.
451453
} else if (fetch_->status_ok()) {
452454
server_context_->rewrite_stats()->ipro_served()->Add(1);
453455
handled = true;
@@ -504,12 +506,12 @@ bool InstawebHandler::HandleAsProxy() {
504506
&host_header, &is_proxy) &&
505507
is_proxy) {
506508
RewriteDriver* driver = MakeDriver();
507-
MakeFetch(mapped_url, "proxy");
509+
MakeFetch(mapped_url, true /* buffered */, "proxy");
508510
fetch_->set_is_proxy(true);
509511
driver->SetRequestHeaders(*fetch_->request_headers());
510512
server_context_->proxy_fetch_factory()->StartNewProxyFetch(
511513
mapped_url, fetch_, driver, NULL, NULL);
512-
handled = WaitForFetch() != ApacheFetch::kAbandonedAndDeclined;
514+
handled = WaitForFetch();
513515
}
514516

515517
return handled; // false == declined
@@ -867,39 +869,51 @@ apr_status_t InstawebHandler::instaweb_handler(request_rec* request) {
867869
server_context->StatisticsPage(is_global_statistics,
868870
instaweb_handler.query_params(),
869871
instaweb_handler.options(),
870-
instaweb_handler.MakeFetch("statistics"));
872+
instaweb_handler.MakeFetch(
873+
false /* unbuffered */, "statistics"));
871874
return OK;
872875
} else if ((request_handler_str == kAdminHandler) ||
873876
(request_handler_str == kGlobalAdminHandler)) {
874877
InstawebHandler instaweb_handler(request);
878+
// The fetch has to be buffered because if it's a cache lookup it could
879+
// complete asynchrously via the rewrite thread.
875880
server_context->AdminPage((request_handler_str == kGlobalAdminHandler),
876881
instaweb_handler.stripped_gurl(),
877882
instaweb_handler.query_params(),
878883
instaweb_handler.options(),
879-
instaweb_handler.MakeFetch("admin"));
884+
instaweb_handler.MakeFetch(
885+
true /* buffered */, "admin"));
880886
ret = OK;
881887
} else if (global_config->enable_cache_purge() &&
882888
!global_config->purge_method().empty() &&
883889
(global_config->purge_method() == request->method)) {
884890
InstawebHandler instaweb_handler(request);
885891
AdminSite* admin_site = server_context->admin_site();
892+
// I'm not convinced that the purge handler must complete synchronously. It
893+
// schedules work on the rewrite driver factory's scheduler, and while in my
894+
// testing it processes everything on the calling thread I'm not sure this
895+
// is part of the contract. The response is just headers and a few bytes of
896+
// body, so buffering is basically free. To be on the safe side let's
897+
// buffer this one too.
886898
admin_site->PurgeHandler(instaweb_handler.original_url_,
887899
server_context->cache_path(),
888-
instaweb_handler.MakeFetch("purge"));
900+
instaweb_handler.MakeFetch(
901+
true /* buffered */, "purge"));
889902
ret = OK;
890903
} else if (request_handler_str == kConsoleHandler) {
891904
InstawebHandler instaweb_handler(request);
892905
server_context->ConsoleHandler(*instaweb_handler.options(),
893906
AdminSite::kOther,
894907
instaweb_handler.query_params(),
895-
instaweb_handler.MakeFetch("console"));
908+
instaweb_handler.MakeFetch(
909+
false /* unbuffered */, "console"));
896910
ret = OK;
897911
} else if (request_handler_str == kMessageHandler) {
898912
InstawebHandler instaweb_handler(request);
899913
server_context->MessageHistoryHandler(
900914
*instaweb_handler.options(),
901915
AdminSite::kOther,
902-
instaweb_handler.MakeFetch("messages"));
916+
instaweb_handler.MakeFetch(false /* unbuffered */, "messages"));
903917
ret = OK;
904918
} else if (request_handler_str == kLogRequestHeadersHandler) {
905919
// For testing CustomFetchHeader.

net/instaweb/apache/instaweb_handler.h

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,16 @@ class InstawebHandler {
9292
// Allocates a Fetch object associated with the current request and
9393
// the specified URL. Include in debug_info anything that's cheap to create
9494
// and would be informative if something went wrong with the fetch.
95-
ApacheFetch* MakeFetch(const GoogleString& url, StringPiece debug_info);
96-
97-
// Allocates a Fetch object associated with the current request and its
98-
// URL.
99-
ApacheFetch* MakeFetch(StringPiece debug_info) {
100-
return MakeFetch(original_url_, debug_info);
95+
// If any uses will be from other threads you must set buffered=true to keep
96+
// your other thread from getting blocked if our output is being read by a
97+
// slow reader.
98+
ApacheFetch* MakeFetch(
99+
const GoogleString& url, bool buffered, StringPiece debug_info);
100+
101+
// Allocates a Fetch object associated with the current request and its URL.
102+
// Please read the comment above before setting buffered=false.
103+
ApacheFetch* MakeFetch(bool buffered, StringPiece debug_info) {
104+
return MakeFetch(original_url_, buffered, debug_info);
101105
}
102106

103107
// Attempts to handle this as a proxied resource (see
@@ -115,13 +119,11 @@ class InstawebHandler {
115119
void HandleAsPagespeedResource();
116120

117121
// Waits for an outstanding fetch (obtained by MakeFetch) to complete or time
118-
// out. Returns kWaitSuccess on success, on timeout it needs to indicate
119-
// whether the request should be declined or counted as handled. If we didn't
120-
// send out headers then it's safe to decline this request and another Apache
121-
// content handler can look into it, so we'll return AbandonedAndDeclined.
122-
// Returning AbandonedAndHandled means we got at least as far as sending out
123-
// headers, and there's nothing else it's safe to do with this request.
124-
ApacheFetch::WaitResult WaitForFetch();
122+
// out. Returns true if the fetch completes, false if we time out and abandon
123+
// the request. On failure we haven't sent out headers or any other content,
124+
// so it's safe to decline this request and another Apache content handler can
125+
// look into it.
126+
bool WaitForFetch();
125127

126128
RequestHeaders* ReleaseRequestHeaders() { return request_headers_.release(); }
127129

0 commit comments

Comments
 (0)