Chromium Code Reviews
Help | Chromium Project | Sign in
(274)

Issue 149525: Remove the concept of threading from ProxyService, and move it into the Proxy... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by eroman
Modified:
2 years, 10 months ago
Reviewers:
willchan
CC:
chromium-reviews_googlegroups.com, John Grabowski, darin, willchan
Visibility:
Public.

Description

Remove the concept of threading from ProxyService, and move it into the ProxyResolver dependency.

ProxyResolver may now complete requests asynchronously, and is defined to handle multiple requests.

The code from ProxyService that queued requests onto the single PAC thread has moved into SingleThreadedProxyResolver.

This refactor lays the groundwork for:

(1) http://crbug.com/11746 -- Run PAC proxy resolving out of process.
(Can inject an IPC bridge implementation of ProxyResolver)


(2) http://crbug.com/11079 -- Run PAC proxy resolving on multiple threads.
(Can implement a MultithreadedProxyResolver type class; still complications around v8 threadsafety though).

BUG=http://crbug.com/11746, http://crbug.com/11079
TEST=existing unit-tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21631

Patch Set 1 #

Patch Set 2 : Add two missing files to the CL #

Patch Set 3 : Fix some style nits #

Patch Set 4 : Moar tests. #

Patch Set 5 : Fix a header ordering #

Total comments: 53

Patch Set 6 : Address willchan's comments #

Patch Set 7 : Add a missing SetPacScriptByUrl() implementation #

Total comments: 24

Patch Set 8 : address willchan's comments #

Total comments: 6

Patch Set 9 : Sync #

Patch Set 10 : Address willchan's comments #

Patch Set 11 : Add missing header for mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+960 lines, -1228 lines) Lint Patch
M chrome/browser/net/resolve_proxy_msg_helper_unittest.cc View 2 3 4 5 6 7 8 7 chunks +29 lines, -9 lines 0 comments 0 errors Download
M net/net.gyp View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver.h View 1 2 3 4 5 6 7 8 2 chunks +53 lines, -20 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_mac.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +19 lines, -4 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_mac.cc View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -3 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_perftest.cc View 2 3 4 5 6 7 8 5 chunks +8 lines, -9 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_v8.h View 1 2 3 4 5 6 7 8 9 2 chunks +11 lines, -6 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_v8.cc View 1 2 3 4 5 6 7 8 9 3 chunks +17 lines, -9 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_v8_unittest.cc View 1 2 3 4 5 6 7 8 15 chunks +20 lines, -20 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_winhttp.h View 1 2 3 4 5 6 7 8 9 2 chunks +16 lines, -6 lines 0 comments 0 errors Download
M net/proxy/proxy_resolver_winhttp.cc View 1 2 3 4 5 6 7 8 4 chunks +14 lines, -4 lines 0 comments 0 errors Download
M net/proxy/proxy_service.h View 1 2 3 4 5 6 7 8 11 chunks +25 lines, -19 lines 0 comments 0 errors Download
M net/proxy/proxy_service.cc View 1 2 3 4 5 6 7 8 9 12 chunks +149 lines, -167 lines 0 comments 1 errors Download
M net/proxy/proxy_service_unittest.cc View 1 2 3 4 5 6 7 8 21 chunks +106 lines, -47 lines 0 comments 0 errors Download
A + net/proxy/single_threaded_proxy_resolver.h View 1 2 3 4 5 6 7 8 9 1 chunk +53 lines, -246 lines 0 comments 1 errors Download
A + net/proxy/single_threaded_proxy_resolver.cc View 1 2 3 4 5 6 7 8 9 2 chunks +120 lines, -659 lines 0 comments 0 errors Download
A net/proxy/single_threaded_proxy_resolver_unittest.cc View 1 2 3 4 5 6 7 1 chunk +313 lines, -0 lines 0 comments 1 errors Download
Commit:

Messages

Total messages: 9
eroman
4 years, 9 months ago #1
eroman
I have updated "single_threaded_proxy_resolver_unittest.cc" with more tests.
4 years, 9 months ago #2
willchan
My brain started melting down when I hit ProxyService and SingletThreadedProxyResolver. I'll take a look ...
4 years, 9 months ago #3
willchan
Took me awhile to get through this chunk, but I think I'm grokking it now. ...
4 years, 9 months ago #4
eroman
http://codereview.chromium.org/149525/diff/89/1075 File net/proxy/proxy_resolver.h (right): http://codereview.chromium.org/149525/diff/89/1075#newcode35 Line 35: // for |url|. If the request will complete ...
4 years, 9 months ago #5
willchan
http://codereview.chromium.org/149525/diff/89/1064 File net/proxy/proxy_service.cc (right): http://codereview.chromium.org/149525/diff/89/1064#newcode47 Line 47: ProxyResolverNull() : ProxyResolver(false /*expects_pac_bytes*/) {} On 2009/07/15 01:40:21, ...
4 years, 9 months ago #6
eroman
http://codereview.chromium.org/149525/diff/89/1066 File net/proxy/single_threaded_proxy_resolver.cc (right): http://codereview.chromium.org/149525/diff/89/1066#newcode216 Line 216: if (it != pending_jobs_.end()) { On 2009/07/17 17:00:04, ...
4 years, 9 months ago #7
willchan
LGTM http://codereview.chromium.org/149525/diff/160/1104 File net/proxy/proxy_resolver_v8.h (right): http://codereview.chromium.org/149525/diff/160/1104#newcode54 Line 54: virtual void SetPacScriptByDataInternal(const std::string& bytes); On 2009/07/20 ...
4 years, 8 months ago #8
eroman
4 years, 8 months ago #9
http://codereview.chromium.org/149525/diff/160/1104
File net/proxy/proxy_resolver_v8.h (right):

http://codereview.chromium.org/149525/diff/160/1104#newcode54
Line 54: virtual void SetPacScriptByDataInternal(const std::string& bytes);
On 2009/07/23 22:40:43, willchan wrote:
> On 2009/07/20 21:22:40, eroman wrote:
> > On 2009/07/17 17:00:04, willchan wrote:
> > > This should be private.
> > 
> > I struggled with this decision -- by splitting it makes it harder to read,
> since
> > the "ProxyResolver implementation" block is split between non-contiguous
> > sections. Which seems more error prone in missing updates if the interface
> > changes (especially since SetPacScriptByDataInternal() is not a required
=0).
> > 
> > How would you feel about me keeping it in this position by adding a middle
> > private section?
> > 
> > Something like:
> > 
> > public:
> > 
> > ...
> > 
> >   // ProxyResolver implementation:
> >   virtual int GetProxyForURL(const GURL& url,
> >                              ProxyInfo* results,
> >                              CompletionCallback* callback,
> >                              RequestHandle* request);
> >   virtual void CancelRequest(RequestHandle request);
> >   virtual void SetPacScriptByUrlInternal(const GURL& pac_url);
> > private:
> >   virtual void SetPacScriptByDataInternal(const std::string& bytes);
> > 
> > public:
> > 
> > ...
> 
> This breaks the ordering of access controls sections as specified by the style
> guide.
> 
> My preference is still just to keep it in the private section.  The way I
think
> of it, it's clear enough that it's part of the interface since it's virtual. 
If
> you're worried about missing updates, then I think pure virtual is the way to
> go.

Done.

http://codereview.chromium.org/149525/diff/160/1098
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/149525/diff/160/1098#newcode369
Line 369: // of one of the requests.
On 2009/07/23 22:40:43, willchan wrote:
> On 2009/07/20 21:22:40, eroman wrote:
> > On 2009/07/17 17:00:04, willchan wrote:
> > > I haven't wrapped my head around this completely.  Can you explain how
it's
> > safe
> > > to continue processing the PendingRequests set after |this| is deleted? 
It
> > > looks like PacRequest::QueryComplete() and PacRequest::QueryDidComplete()
> both
> > > call back to service_, which would be deleted memory then, right?
> > 
> > If |this| is deleted, then all of the PacRequest instances are Cancel()-ed.
> > 
> > Since PacRequest is reference counted, |pending_copy| holds a reference to
> these
> > same request objects, which will survive the deletion of |this|.
> > 
> > When iterating through them now, the check for |!req->was_cancelled()|
> prevents
> > processing requests from the deleted ProxyService.
> > 
> > (in other words, the deletion case generalizes to cancellation case).
> 
> Ah, that makes sense now.  Can you add the "If |this| is deleted, then all of
> the PacRequest instances are Cancel()-ed." to the above comment?

Done.

http://codereview.chromium.org/149525/diff/2001/1126
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/149525/diff/2001/1126#newcode63
Line 63: coordinator_->thread()->message_loop()->PostTask(FROM_HERE,
On 2009/07/23 22:40:43, willchan wrote:
> The indentation of the arguments is off.  FROM_HERE probably should move to
the
> next line.

Done.

http://codereview.chromium.org/149525/diff/2001/1126#newcode84
Line 84: DCHECK(rv != ERR_IO_PENDING);
On 2009/07/23 22:40:43, willchan wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/149525/diff/2001/1126#newcode171
Line 171: // It is here to make diffs against proxy_service.cc cleaner.
On 2009/07/23 22:40:43, willchan wrote:
> You can do this now if you want since I've already read through it.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6