Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14813024:
Introduce RequestWebSocketStream into HttpStreamFactory (Closed)
|
Created:
7 years, 7 months ago by yhirano Modified:
7 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionIntroduce RequestWebSocketStream into HttpStreamFactory
Introduce RequestWebSocketStream into HttpStreamFactory to reuse its functionality that handles socket pool, proxy and SSL.
BUG=237444
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207735
Patch Set 1 #
Total comments: 8
Patch Set 2 : rebase #Patch Set 3 : #Patch Set 4 : Fix a comment. #
Total comments: 16
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : #
Total comments: 12
Patch Set 8 : #
Total comments: 19
Patch Set 9 : rebase #Patch Set 10 : #Patch Set 11 : Rename delegate functions. #
Total comments: 17
Patch Set 12 : rebase #Patch Set 13 : #Patch Set 14 : #
Total comments: 15
Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #
Total comments: 44
Patch Set 18 : #Patch Set 19 : #
Total comments: 19
Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : rebase #
Total comments: 24
Patch Set 25 : #Patch Set 26 : #Patch Set 27 : #
Total comments: 22
Patch Set 28 : #Patch Set 29 : #Patch Set 30 : #Patch Set 31 : rebase #
Total comments: 32
Patch Set 32 : #Patch Set 33 : #Patch Set 34 : #Patch Set 35 : rebase #Patch Set 36 : rebase #
Total comments: 8
Patch Set 37 : #
Total comments: 16
Patch Set 38 : #Patch Set 39 : #
Total comments: 4
Patch Set 40 : #Messages
Total messages: 65 (0 generated)
Hi, can you review this CL? I gave up casting out "WebSocket" from the interface and the code, because I couldn't reason the logic without the word. If you give me L-G-T-M's, I will send this CL to net reviewers.
https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_job.cc:318: spdy_session_to_pass_ = NULL; these lines need to be placed out of the following if block different from OnSocketReadyCallback and OnStreamReadyCallback? just wondering. it looks ok. https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_request.h:15: namespace net { blank line please https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_unittest.cc:36: namespace { blank line https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_unittest.cc:105: ; bool waiting_for_preconnect_; typo?
https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_job.cc:318: spdy_session_to_pass_ = NULL; On 2013/05/13 11:04:44, tyoshino wrote: > these lines need to be placed out of the following if block different from > OnSocketReadyCallback and OnStreamReadyCallback? > > just wondering. it looks ok. This code is differenct from OnSocketReadyCallback and OnstreamReadyCallback because connection_ and stream_ is a owned by this object and is a scoped_ptr respectively, but spdy_session_to_pass_ is not. I got the idea from OnSpdySessionReadyCallback. https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_request.h:15: namespace net { On 2013/05/13 11:04:44, tyoshino wrote: > blank line please Done. https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_unittest.cc:36: namespace { On 2013/05/13 11:04:44, tyoshino wrote: > blank line Done. https://codereview.chromium.org/14813024/diff/1/net/http/http_stream_factory_... net/http/http_stream_factory_impl_unittest.cc:105: ; bool waiting_for_preconnect_; On 2013/05/13 11:04:44, tyoshino wrote: > typo? Thanks, done.
https://codereview.chromium.org/14813024/diff/13001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_network_tra... net/http/http_network_transaction.cc:428: void HttpNetworkTransaction::OnSocketReady(const SSLConfig& used_ssl_config, I think this implementation of OnSocketReady() and OnSpdySessionReady() should not be called. Correct? It might be good to put NOTREACHED() << "This implementation of OnSocketReady should never be called"; or similar in here and OnSpdySessionReady() below. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:72: // This is another success case. How about "This is a success case for WebSockets" ? https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:73: // Some factory can call this function instead of OnStreamReady. It is probably better to specifically say RequestStreamForWebSocket instead of "some factory" here. Also saying "calls" is cleaner than saying "can call". https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:88: // You can create and hold a scoped_refptr pointer to retain it. Better to say "you should" rather than "you can" here, I think. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:210: // Request a stream part for a websocket connection. I do not understand the word "part" in this context. I think you can just remove it. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1089: spdy_session_to_pass_ = new_spdy_session_; It is probably a tiny bit more efficient to do new_spdy_session_.swap(spdy_session_to_pass_); here (and below). https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:209: ssl_config_service = new SSLConfigServiceDefaults(); This looks like the MockSSLConfigService is never used. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:599: // Now request a stream. It should succeed using the second proxy in the Doesn't ProxyService::CreateDirect() above imply that this connection will not use a proxy?
https://codereview.chromium.org/14813024/diff/13001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_network_tra... net/http/http_network_transaction.cc:428: void HttpNetworkTransaction::OnSocketReady(const SSLConfig& used_ssl_config, On 2013/05/13 13:03:03, Adam Rice wrote: > I think this implementation of OnSocketReady() and OnSpdySessionReady() should > not be called. Correct? > > It might be good to put > > NOTREACHED() << "This implementation of OnSocketReady should never be called"; > > or similar in here and OnSpdySessionReady() below. You are right, done. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:72: // This is another success case. On 2013/05/13 13:03:03, Adam Rice wrote: > How about "This is a success case for WebSockets" ? Done. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:73: // Some factory can call this function instead of OnStreamReady. On 2013/05/13 13:03:03, Adam Rice wrote: > It is probably better to specifically say RequestStreamForWebSocket instead of > "some factory" here. > > Also saying "calls" is cleaner than saying "can call". I deleted this line. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:88: // You can create and hold a scoped_refptr pointer to retain it. On 2013/05/13 13:03:03, Adam Rice wrote: > Better to say "you should" rather than "you can" here, I think. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory.h:210: // Request a stream part for a websocket connection. On 2013/05/13 13:03:03, Adam Rice wrote: > I do not understand the word "part" in this context. I think you can just remove > it. Done. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1089: spdy_session_to_pass_ = new_spdy_session_; On 2013/05/13 13:03:03, Adam Rice wrote: > It is probably a tiny bit more efficient to do > > new_spdy_session_.swap(spdy_session_to_pass_); > > here (and below). Done. spdy_session_to_pass must be NULL here. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:209: ssl_config_service = new SSLConfigServiceDefaults(); On 2013/05/13 13:03:03, Adam Rice wrote: > This looks like the MockSSLConfigService is never used. Thanks, done. https://codereview.chromium.org/14813024/diff/13001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:599: // Now request a stream. It should succeed using the second proxy in the On 2013/05/13 13:03:03, Adam Rice wrote: > Doesn't ProxyService::CreateDirect() above imply that this connection will not > use a proxy? Thanks, I copied from the comment from the above test case. Done.
lgtm
https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1029: *http_pipelining_key_.get())); have you checked that a request for ws doesn't enter this path? https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1071: connection_->Reset(); if it goes this path, connection_ is abandoned but not null, and therefore, just checking connection_ at L519 is not sufficient? https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.h:313: // a spdy session to pass to OnSpdySessionReadyForWSCallback a spdy -> A SPDY
https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1029: *http_pipelining_key_.get())); On 2013/05/15 04:29:34, tyoshino wrote: > have you checked that a request for ws doesn't enter this path? No, I haven't. I modified IsRequestEligibleForPipelining so that HTTP pipeline can't be used if request->for_websocket() is true. https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1071: connection_->Reset(); On 2013/05/15 04:29:34, tyoshino wrote: > if it goes this path, connection_ is abandoned but not null, and therefore, just > checking connection_ at L519 is not sufficient? You are right. I added a check for connection_->is_initialized(). https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/17001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.h:313: // a spdy session to pass to OnSpdySessionReadyForWSCallback On 2013/05/15 04:29:34, tyoshino wrote: > a spdy -> A SPDY Done.
https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:159: } factor out common part? https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:23: #include "net/socket/mock_client_socket_pool_manager.h" include net/socket/next_proto.h https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:27: #include "net/spdy/spdy_test_util_spdy3.h" what is thi for? maybe spdy_test_util_common.h? https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:45: } (optional) it would be more useful if the original values are saved and restored. not necessary now, so just optional suggestion. https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:585: MockRead r(ASYNC, OK); r -> mock_read https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:627: MockRead r(ASYNC, OK); ditto
https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:159: } On 2013/05/15 07:48:52, tyoshino wrote: > factor out common part? Done. https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:23: #include "net/socket/mock_client_socket_pool_manager.h" On 2013/05/15 07:48:52, tyoshino wrote: > include net/socket/next_proto.h Done. https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:27: #include "net/spdy/spdy_test_util_spdy3.h" On 2013/05/15 07:48:52, tyoshino wrote: > what is thi for? > > maybe spdy_test_util_common.h? Done. https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:45: } On 2013/05/15 07:48:52, tyoshino wrote: > (optional) it would be more useful if the original values are saved and > restored. not necessary now, so just optional suggestion. Done. https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:585: MockRead r(ASYNC, OK); On 2013/05/15 07:48:52, tyoshino wrote: > r -> mock_read Done. https://codereview.chromium.org/14813024/diff/38001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:627: MockRead r(ASYNC, OK); On 2013/05/15 07:48:52, tyoshino wrote: > ditto Done.
LGTM
willchan, can you take a look at this CL? This CL intends to add a RequestStream-like method to HttpStreamFactory for the sake of WebSocket connection initialization. Thanks,
+mmenke
A couple very preliminary comments. It is going to take me a while to fully wrap my head around just how this integrates into things, so I'm going to be slow on this. Apologies in advance. As I recall, the plan is to have separate socket pools for WebSocket and non-WebSocket requests, so I guess WebSockets and HTTP requests will not share SPDY sessions? https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:280: void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() { DCHECK(!request_->for_websocket()); https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.h:130: void OnSocketReadyCallback(); nit: You have the socket function first everywhere else, best to be consistent. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:315: void HttpStreamFactoryImpl::Request::OnSpdySessionReady( It looks to me like this path, which I believe is used for getting existing SPDY sessions, is not correct in the new case. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.h:77: void OnSpdySessionReadyForWS(Job* job, I think the code is clearer if we don't abbreviate websocket. Does make for longer names, but I think the clarity is worth the ugliness and extra line breaks. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.h:84: ClientSocketHandle* connection); I think we need to decide if we're going to consider this a generic way to get a lower-level connection object, or if this is specific for websockets. In the latter case, all of these should have WebSocket in the title. In the latter, they should not. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:37: class HttpStreamFactorySpdyForcer { Suggest ScopedForceSpdyOverSsl. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:49: private: nit: Line break before private https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:51: const bool always_; I suggest orig_force_spdy_over_ssl_ / orig_force_spdy_always_. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:628: TEST(HttpStreamFactoryTest, RequestSpdySession) { Maybe a test where there's a pre-existing SPDY session?
I think using separate SPDY sessions is acceptable in the short-term. Once we have that working, sharing existing SPDY sessions would be an obvious improvement. I would also like to be able to steal connections that were speculatively created for HTTP but haven't actually been used, but again that's an optimisation and not something I think belongs in the initial implementation. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.h:84: ClientSocketHandle* connection); On 2013/05/15 15:38:20, mmenke wrote: > I think we need to decide if we're going to consider this a generic way to get a > lower-level connection object, or if this is specific for websockets. > > In the latter case, all of these should have WebSocket in the title. In the > latter, they should not. Unless we have another use case on the table, I would like to keep it WebSocket-only for now. It would be easier to rename and generalise these interfaces in the future that it would be to try to predict the future now.
Thank you for the comments. > As I recall, the plan is to have separate socket pools for WebSocket and non-WebSocket requests, so I guess WebSockets and HTTP requests will not share SPDY sessions? There is a filed bug http://crbug.com/118268 for that. I would do that in another CL. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:280: void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() { On 2013/05/15 15:38:20, mmenke wrote: > DCHECK(!request_->for_websocket()); Done. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.h:130: void OnSocketReadyCallback(); On 2013/05/15 15:38:20, mmenke wrote: > nit: You have the socket function first everywhere else, best to be consistent. Done. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:315: void HttpStreamFactoryImpl::Request::OnSpdySessionReady( On 2013/05/15 15:38:20, mmenke wrote: > It looks to me like this path, which I believe is used for getting existing SPDY > sessions, is not correct in the new case. This function will not be called when for_websocket() is true. So There is no problem but naming confusion, I think. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.h:77: void OnSpdySessionReadyForWS(Job* job, On 2013/05/15 15:38:20, mmenke wrote: > I think the code is clearer if we don't abbreviate websocket. Does make for > longer names, but I think the clarity is worth the ugliness and extra line > breaks. Done. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.h:84: ClientSocketHandle* connection); On 2013/05/16 04:00:36, Adam Rice wrote: > On 2013/05/15 15:38:20, mmenke wrote: > > I think we need to decide if we're going to consider this a generic way to get > a > > lower-level connection object, or if this is specific for websockets. > > > > In the latter case, all of these should have WebSocket in the title. In the > > latter, they should not. > > Unless we have another use case on the table, I would like to keep it > WebSocket-only for now. It would be easier to rename and generalise these > interfaces in the future that it would be to try to predict the future now. I see, I renamed the delegate functions. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:37: class HttpStreamFactorySpdyForcer { On 2013/05/15 15:38:20, mmenke wrote: > Suggest ScopedForceSpdyOverSsl. Done. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:49: private: On 2013/05/15 15:38:20, mmenke wrote: > nit: Line break before private Done. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:51: const bool always_; On 2013/05/15 15:38:20, mmenke wrote: > I suggest orig_force_spdy_over_ssl_ / orig_force_spdy_always_. Done. https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:628: TEST(HttpStreamFactoryTest, RequestSpdySession) { On 2013/05/15 15:38:20, mmenke wrote: > Maybe a test where there's a pre-existing SPDY session? Done.
I'm really sorry for being so slow on this - I did not anticipate taking this long. I'll get back to you tonight or tomorrow. On 2013/05/16 05:02:34, yhirano wrote: > Thank you for the comments. > > > As I recall, the plan is to have separate socket pools for WebSocket and > non-WebSocket requests, so I guess WebSockets and HTTP requests will not share > SPDY sessions? > There is a filed bug http://crbug.com/118268 for that. > I would do that in another CL. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_job.cc:280: void > HttpStreamFactoryImpl::Job::OnStreamReadyCallback() { > On 2013/05/15 15:38:20, mmenke wrote: > > DCHECK(!request_->for_websocket()); > > Done. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > File net/http/http_stream_factory_impl_job.h (right): > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_job.h:130: void OnSocketReadyCallback(); > On 2013/05/15 15:38:20, mmenke wrote: > > nit: You have the socket function first everywhere else, best to be > consistent. > > Done. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > File net/http/http_stream_factory_impl_request.cc (right): > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_request.cc:315: void > HttpStreamFactoryImpl::Request::OnSpdySessionReady( > On 2013/05/15 15:38:20, mmenke wrote: > > It looks to me like this path, which I believe is used for getting existing > SPDY > > sessions, is not correct in the new case. > This function will not be called when for_websocket() is true. So There is no > problem but naming confusion, I think. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > File net/http/http_stream_factory_impl_request.h (right): > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_request.h:77: void > OnSpdySessionReadyForWS(Job* job, > On 2013/05/15 15:38:20, mmenke wrote: > > I think the code is clearer if we don't abbreviate websocket. Does make for > > longer names, but I think the clarity is worth the ugliness and extra line > > breaks. > > Done. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_request.h:84: ClientSocketHandle* connection); > On 2013/05/16 04:00:36, Adam Rice wrote: > > On 2013/05/15 15:38:20, mmenke wrote: > > > I think we need to decide if we're going to consider this a generic way to > get > > a > > > lower-level connection object, or if this is specific for websockets. > > > > > > In the latter case, all of these should have WebSocket in the title. In the > > > latter, they should not. > > > > Unless we have another use case on the table, I would like to keep it > > WebSocket-only for now. It would be easier to rename and generalise these > > interfaces in the future that it would be to try to predict the future now. > I see, I renamed the delegate functions. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > File net/http/http_stream_factory_impl_unittest.cc (right): > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_unittest.cc:37: class > HttpStreamFactorySpdyForcer { > On 2013/05/15 15:38:20, mmenke wrote: > > Suggest ScopedForceSpdyOverSsl. > > Done. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_unittest.cc:49: private: > On 2013/05/15 15:38:20, mmenke wrote: > > nit: Line break before private > > Done. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_unittest.cc:51: const bool always_; > On 2013/05/15 15:38:20, mmenke wrote: > > I suggest orig_force_spdy_over_ssl_ / orig_force_spdy_always_. > > Done. > > https://codereview.chromium.org/14813024/diff/43001/net/http/http_stream_fact... > net/http/http_stream_factory_impl_unittest.cc:628: TEST(HttpStreamFactoryTest, > RequestSpdySession) { > On 2013/05/15 15:38:20, mmenke wrote: > > Maybe a test where there's a pre-existing SPDY session? > > Done.
https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory.h:208: // Request a stream for a websocket connection. We're not actually requesting an HttpStream, but rather a ClientSocketHandle that could be used for a WebSocket, or a SpdySession that can be used to create WebSocket. Is there a reason we can't just create a WebSocketStream (Or whatever) and return that to the delegate? https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:30: explicit HttpStreamFactoryImpl(HttpNetworkSession* session); An HttpStreamFactory either is for WebSockets, or is for HttpStreams. It's not currently possible to mix-and-match. Since this is a pretty important property that would take a lot of work to work around (I'm also not sure if a socket used for an HTTP stream can be switched over to being used for WebSockets, per spec), I think it makes sense to take in a boolean indicating which we're making, and DCHECK on it when requesting a stream. Could merge RequestStream functions as well, though I'm not sure that's a good idea. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:300: DCHECK(!IsPreconnecting()); Looks like when we're preconnected, request_ is NULL, so this should probably go before we dereference request_. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:321: stream_factory_->OnOrphanedJobComplete(this); This is what was confusing me before - we don't call OnNewSpdySessionReady here. So if we're orphaned, and there's another request out for the same SpdySession, when we're here, don't make the session available for the other request. This seems like a problem. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:536: return ERR_FAILED; We should fail asynchronously in this case, just like in the default case. Notice that HttpStreamFactoryImpl::Job::OnIOComplete() doesn't actually do anything with the result, so this function always has to return ERR_IO_PENDING (Which, admittedly, makes having a return value a little weird, but that's beyond the scope of this CL). https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1038: // connection_ will be passed to the delegate. Not creating a stream in DoCreateStream in this case seems a bit odd. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:129: DCHECK(completed_); I'd like a some more DCHECKs on for_websocket_ in these functions. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:320: bool direct) { For functions that cannot be called when using websockets, I'd like to see DCHECKs for documentation purposes.
https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory.h:208: // Request a stream for a websocket connection. On 2013/05/23 20:26:20, mmenke wrote: > We're not actually requesting an HttpStream, but rather a ClientSocketHandle > that could be used for a WebSocket, or a SpdySession that can be used to create > WebSocket. Is there a reason we can't just create a WebSocketStream (Or > whatever) and return that to the delegate? iOS Chrome compiles libnet without WebSocket support. So, WebSocketStream is not necessarily available. While of course we could make it available as a special case, I think it is preferable to minimise the amount of WebSocket-related code that needs to be compiled in when WebSockets are compiled out.
On 2013/05/24 03:41:09, Adam Rice wrote: > https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... > File net/http/http_stream_factory.h (right): > > https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... > net/http/http_stream_factory.h:208: // Request a stream for a websocket > connection. > On 2013/05/23 20:26:20, mmenke wrote: > > We're not actually requesting an HttpStream, but rather a ClientSocketHandle > > that could be used for a WebSocket, or a SpdySession that can be used to > create > > WebSocket. Is there a reason we can't just create a WebSocketStream (Or > > whatever) and return that to the delegate? > > iOS Chrome compiles libnet without WebSocket support. So, WebSocketStream is not > necessarily available. While of course we could make it available as a special > case, I think it is preferable to minimise the amount of WebSocket-related code > that needs to be compiled in when WebSockets are compiled out. It wouldn't be hard to just ifdef out a couple lines based on ENABLE_WEBSOCKET (Which would be false on iOS). Another option would be to move the stream/socket creation to the Delegate, which would just have methods to take a session or socket. One Delegate subclass would create HttpStreams, the other (Not compiled on iOS) would create WebSocketStreams.
Hi, Matt, thank you very much for your comments. There are some design changes in response to your comments. 1. I added WebSocketStreamBase on net/http. It is the base class of WebSocketStream and has no dependency with net/websockets. We use this class to handle WebSocketStream objects in net/http code. 2. for_websocket belongs HttpStreamFactoryImpl, not HttpStreamFactoryImpl::Request now. So You can't call RequestStream with a HttpStreamFactoryImpl with for_websocket = true and vice versa. Since we want to share same HttpNetworkSession with other http connections, I add websocket_stream_factory_ member to HttpNetworkSession. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory.h:208: // Request a stream for a websocket connection. On 2013/05/24 03:41:09, Adam Rice wrote: > On 2013/05/23 20:26:20, mmenke wrote: > > We're not actually requesting an HttpStream, but rather a ClientSocketHandle > > that could be used for a WebSocket, or a SpdySession that can be used to > create > > WebSocket. Is there a reason we can't just create a WebSocketStream (Or > > whatever) and return that to the delegate? > > iOS Chrome compiles libnet without WebSocket support. So, WebSocketStream is not > necessarily available. While of course we could make it available as a special > case, I think it is preferable to minimise the amount of WebSocket-related code > that needs to be compiled in when WebSockets are compiled out. I talked with Adam and we agreed to add WebSocketStreamBase class which is the base class of WebSocketStream. WebSocketStreamBase is placed on net/http and has no dependency to net/websockets. When net/websockets code get a WebSocketStreamBase, it should downcast the object into a derived class depending on the object's type() property. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:30: explicit HttpStreamFactoryImpl(HttpNetworkSession* session); On 2013/05/23 20:26:20, mmenke wrote: > An HttpStreamFactory either is for WebSockets, or is for HttpStreams. It's not > currently possible to mix-and-match. Since this is a pretty important property > that would take a lot of work to work around (I'm also not sure if a socket used > for an HTTP stream can be switched over to being used for WebSockets, per spec), > I think it makes sense to take in a boolean indicating which we're making, and > DCHECK on it when requesting a stream. > > Could merge RequestStream functions as well, though I'm not sure that's a good > idea. I see, I moved for_websocket_ member from HttpStreamFactory::request to HttpStreamFactory. Now an HttpStreamFactory can handle either RequestStream or RequestWebSocketStream, not both, so I add websocket_stream_factory_ member and its accessor to HttpNetworkSession class. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:300: DCHECK(!IsPreconnecting()); On 2013/05/23 20:26:20, mmenke wrote: > Looks like when we're preconnected, request_ is NULL, so this should probably go > before we dereference request_. Done. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:321: stream_factory_->OnOrphanedJobComplete(this); On 2013/05/23 20:26:20, mmenke wrote: > This is what was confusing me before - we don't call OnNewSpdySessionReady here. > So if we're orphaned, and there's another request out for the same SpdySession, > when we're here, don't make the session available for the other request. This > seems like a problem. In PS 13 (or newer), OnNewSpdySessionReady is called when a new spdy session is generated regardless of for_websocket value. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:536: return ERR_FAILED; On 2013/05/23 20:26:20, mmenke wrote: > We should fail asynchronously in this case, just like in the default case. > Notice that HttpStreamFactoryImpl::Job::OnIOComplete() doesn't actually do > anything with the result, so this function always has to return ERR_IO_PENDING > (Which, admittedly, makes having a return value a little weird, but that's > beyond the scope of this CL). Done. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:1038: // connection_ will be passed to the delegate. On 2013/05/23 20:26:20, mmenke wrote: > Not creating a stream in DoCreateStream in this case seems a bit odd. Done. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:129: DCHECK(completed_); On 2013/05/23 20:26:20, mmenke wrote: > I'd like a some more DCHECKs on for_websocket_ in these functions. Done. https://codereview.chromium.org/14813024/diff/29012/net/http/http_stream_fact... net/http/http_stream_factory_impl_request.cc:320: bool direct) { On 2013/05/23 20:26:20, mmenke wrote: > For functions that cannot be called when using websockets, I'd like to see > DCHECKs for documentation purposes. In PS13 (or newer), OnNewSpdySessionReady is called even for_websocket is true.
Two fairly significant comments - I intend to to continue reviewing and get back to you with more comments on the rest of the CL, but I have a fairly high priority issue I'm investigating, and don't want to block you in the meantime. https://codereview.chromium.org/14813024/diff/88001/net/http/http_network_ses... File net/http/http_network_session.h (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_network_ses... net/http/http_network_session.h:9: #include <string> While you're here, mind adding a blank line between the standard headers and the others? Thanks! https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:32: // You can call RequestWebSocketStream if and only if |for_websocket| is true. nit: Some people feel that "you" shouldn't be used in comments, since it's somewhat ambiguous. I suggest just using passing voice, something along the lines of "RequestStream may only be called if |for_websocket| is false...", etc. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:34: bool for_websocket); nit: Explicit not needed, since we have two arguments now. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:34: bool for_websocket); Think "for_websockets" may be a little clearer. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:524: result)); Calling OnStreamFailedCallback with a result of OK is a little weird, and could break other classes if they are passed OK in the case of an error. This shouldn't happen, right? Then I'd suggest a comment along those lines, and using ERR_UNEXPECTED. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:833: return InitSocketHandleForHttpRequest( This currently always uses the HttpNetworkSession's normal_socket_pool_manager_, rather than using the websocket_socket_pool_manager_. I'm not sure how involved this will be. It may be simpler to create a separate CL for this, and land it separately. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:722: TEST(HttpStreamFactoryTest, RequestWebSocketBasicStream) { These tests should make sure the socket was created in the right socket pool. Should probably also have proxy tests, to make sure the right socket pools are used.
https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:524: result)); On 2013/05/24 14:53:22, mmenke wrote: > Calling OnStreamFailedCallback with a result of OK is a little weird, and could > break other classes if they are passed OK in the case of an error. > > This shouldn't happen, right? Then I'd suggest a comment along those lines, and > using ERR_UNEXPECTED. On second thought, perhaps just a DCHECK instead of a branch.
I noticed that SSL was not used for "wss//..." uris and I fixed it. http_stream_factory_impl_job.cc:718 and client_socket_pool_manager.cc . https://codereview.chromium.org/14813024/diff/88001/net/http/http_network_ses... File net/http/http_network_session.h (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_network_ses... net/http/http_network_session.h:9: #include <string> On 2013/05/24 14:53:22, mmenke wrote: > While you're here, mind adding a blank line between the standard headers and the > others? Thanks! Done. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:32: // You can call RequestWebSocketStream if and only if |for_websocket| is true. On 2013/05/24 14:53:22, mmenke wrote: > nit: Some people feel that "you" shouldn't be used in comments, since it's > somewhat ambiguous. I suggest just using passing voice, something along the > lines of "RequestStream may only be called if |for_websocket| is false...", etc. Done. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:34: bool for_websocket); On 2013/05/24 14:53:22, mmenke wrote: > nit: Explicit not needed, since we have two arguments now. Done. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl.h:34: bool for_websocket); On 2013/05/24 14:53:22, mmenke wrote: > Think "for_websockets" may be a little clearer. Done. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:524: result)); On 2013/05/24 16:44:43, mmenke wrote: > On 2013/05/24 14:53:22, mmenke wrote: > > Calling OnStreamFailedCallback with a result of OK is a little weird, and > could > > break other classes if they are passed OK in the case of an error. > > > > This shouldn't happen, right? Then I'd suggest a comment along those lines, > and > > using ERR_UNEXPECTED. > > On second thought, perhaps just a DCHECK instead of a branch. Done. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_job.cc:833: return InitSocketHandleForHttpRequest( On 2013/05/24 14:53:22, mmenke wrote: > This currently always uses the HttpNetworkSession's normal_socket_pool_manager_, > rather than using the websocket_socket_pool_manager_. > > I'm not sure how involved this will be. It may be simpler to create a separate > CL for this, and land it separately. Yes, I will do that in another CL. https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/88001/net/http/http_stream_fact... net/http/http_stream_factory_impl_unittest.cc:722: TEST(HttpStreamFactoryTest, RequestWebSocketBasicStream) { On 2013/05/24 14:53:22, mmenke wrote: > These tests should make sure the socket was created in the right socket pool. > > Should probably also have proxy tests, to make sure the right socket pools are > used. Done. Since net::InitSocketHandleForHttp is responsible for the pool selection, we only check parameters for the function. I may modify InitSocketHandleForHttp parameters in a future CL.
This is looking really good to me. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory.h:81: WebSocketStreamBase* stream) = 0; nit: 4 space indent. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory.h:185: // Will call delegate->OnStreamReady upon completion. nit: "on successful completion" https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory.h:195: // Will call delegate->OnWebSocketStreamReady upon completion. nit: "on successful completion" https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:367: return ::net::InitSocketHandleForHttpRequest( nit: leading "::" not needed. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:67: // For testing purpose nit: Add period. Also should be "purposes". https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:68: virtual int InitSocketHandleForHttpRequest( This should be protected. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:253: scoped_refptr<SpdySession> spdy_session_; These two are no longer in use. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:807: EXPECT_TRUE(NULL == waiter.connection()); There's no code to initialize these. Should be checking the websocket_stream instead. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:811: http_stream_factory->calls_init_socket_handle_for_http[0]; Is all this extra infrastructure necessary? The other tests just use things like: GetSocketPoolGroupCount(session->GetSSLSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL)). Seems like we could do the same, except for the WebSocket pools (They're currently using the normal pools, of course. Think it may be a good idea to land a CL to fix that first.) This also serves to test the behavior of InitSocketHandleForHttpRequest, as it doesn't have its own unit tests. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:10: // you should not introduce dependency to net/websockets in this file. My suggestion is just to put this in net/websockets. Since it's a header, excluding it won't break the build. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:21: // without linking net/websockets code. Not really following the second sentence here. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:29: // correspond to WebSockeSpdyStream nit: Comments should generally start with a capital and end with a period. Also, should be "Corresponds" (There's an implied "this" at the start of the sentence, so it is "[This] corresponds to ..." https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:32: kStreamTypeTest Prefer not to add this unless / until we need it. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:35: // You can derive this class and implement each factory method. Comment doesn't add anything useful. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:36: class Factory { Long term, I'd like to get rid of this class, and just use if-defs on platforms without WebSockets enabled. Passing around the factory for every call seems a bit weird. Could also just pass on construction, but think it's better to just get rid of it. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:50: virtual ~Factory() {} The destructor should go first (It could also be protected, but think we may be destroying the virtual class directly). https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:57: virtual ~WebSocketStreamBase() { } nit: Google style guide says no horizontal whitespace in empty function braces. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:60: explicit WebSocketStreamBase(StreamType type): type_(type) {} Mind de-inlining this? https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:60: explicit WebSocketStreamBase(StreamType type): type_(type) {} nit: Space before the colon. https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... net/socket/client_socket_pool_manager.cc:92: request_url.SchemeIs("wss") || force_spdy_over_ssl; Another issue for the CL to use the right HttpStreamFactory: If we're using an HTTP proxy, we want to make sure we use it as a Connect proxy. I believe the current code will just connect to the HTTP proxy, and pass that on to the WebSocketStream.
https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory.h:81: WebSocketStreamBase* stream) = 0; On 2013/05/28 21:22:37, mmenke wrote: > nit: 4 space indent. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory.h:185: // Will call delegate->OnStreamReady upon completion. On 2013/05/28 21:22:37, mmenke wrote: > nit: "on successful completion" Done. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory.h:195: // Will call delegate->OnWebSocketStreamReady upon completion. On 2013/05/28 21:22:37, mmenke wrote: > nit: "on successful completion" Done. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:367: return ::net::InitSocketHandleForHttpRequest( On 2013/05/28 21:22:37, mmenke wrote: > nit: leading "::" not needed. Deleted. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:67: // For testing purpose On 2013/05/28 21:22:37, mmenke wrote: > nit: Add period. Also should be "purposes". Deleted. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:68: virtual int InitSocketHandleForHttpRequest( On 2013/05/28 21:22:37, mmenke wrote: > This should be protected. Deleted. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:253: scoped_refptr<SpdySession> spdy_session_; On 2013/05/28 21:22:37, mmenke wrote: > These two are no longer in use. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:807: EXPECT_TRUE(NULL == waiter.connection()); On 2013/05/28 21:22:37, mmenke wrote: > There's no code to initialize these. Should be checking the websocket_stream > instead. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:811: http_stream_factory->calls_init_socket_handle_for_http[0]; On 2013/05/28 21:22:37, mmenke wrote: > Is all this extra infrastructure necessary? The other tests just use things > like: > GetSocketPoolGroupCount(session->GetSSLSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL)). > > Seems like we could do the same, except for the WebSocket pools (They're > currently using the normal pools, of course. Think it may be a good idea to > land a CL to fix that first.) > > This also serves to test the behavior of InitSocketHandleForHttpRequest, as it > doesn't have its own unit tests. Thank you, you are right. I deleted HttpStreamFactoryImpl::InitSocketHandle and MockHttpStreamFactoryImpl, and use GetSocketPoolGroupCount and used_proxy_info. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:10: // you should not introduce dependency to net/websockets in this file. On 2013/05/28 21:22:37, mmenke wrote: > My suggestion is just to put this in net/websockets. Since it's a header, > excluding it won't break the build. Currently, files in net/websockets are excluded in net/net.gyp if enable_websockets != 1. I think it is difficult to re-include individual files. And, now we have net/http/websocket_stream_base.cc in response to your comment. So I would like to place websocket_stream_base.{h, cc} in net/http. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:21: // without linking net/websockets code. On 2013/05/28 21:22:37, mmenke wrote: > Not really following the second sentence here. Done? https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:29: // correspond to WebSockeSpdyStream On 2013/05/28 21:22:37, mmenke wrote: > nit: Comments should generally start with a capital and end with a period. > Also, should be "Corresponds" (There's an implied "this" at the start of the > sentence, so it is "[This] corresponds to ..." Done. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:32: kStreamTypeTest On 2013/05/28 21:22:37, mmenke wrote: > Prefer not to add this unless / until we need it. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:35: // You can derive this class and implement each factory method. On 2013/05/28 21:22:37, mmenke wrote: > Comment doesn't add anything useful. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:36: class Factory { On 2013/05/28 21:22:37, mmenke wrote: > Long term, I'd like to get rid of this class, and just use if-defs on platforms > without WebSockets enabled. Passing around the factory for every call seems a > bit weird. Could also just pass on construction, but think it's better to just > get rid of it. Likely enough. Or, perhaps we may be able to decouple WebSocketStreamFactory and move it into net/websockets someday. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:50: virtual ~Factory() {} On 2013/05/28 21:22:37, mmenke wrote: > The destructor should go first (It could also be protected, but think we may be > destroying the virtual class directly). Done. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:57: virtual ~WebSocketStreamBase() { } On 2013/05/28 21:22:37, mmenke wrote: > nit: Google style guide says no horizontal whitespace in empty function braces. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:60: explicit WebSocketStreamBase(StreamType type): type_(type) {} On 2013/05/28 21:22:37, mmenke wrote: > nit: Space before the colon. Done. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:60: explicit WebSocketStreamBase(StreamType type): type_(type) {} On 2013/05/28 21:22:37, mmenke wrote: > Mind de-inlining this? Done. https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... net/socket/client_socket_pool_manager.cc:92: request_url.SchemeIs("wss") || force_spdy_over_ssl; On 2013/05/28 21:22:37, mmenke wrote: > Another issue for the CL to use the right HttpStreamFactory: If we're using an > HTTP proxy, we want to make sure we use it as a Connect proxy. I believe the > current code will just connect to the HTTP proxy, and pass that on to the > WebSocketStream. I'm not sure if I understand your comment correctly, but we use CONNECT when we use a proxy server, as RFC6455 says in 4.1, _Proxy Usage_ (http://tools.ietf.org/html/rfc6455#section-4.1). Is that resolve the problem?
https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:10: // you should not introduce dependency to net/websockets in this file. On 2013/05/30 04:44:32, yhirano wrote: > On 2013/05/28 21:22:37, mmenke wrote: > > My suggestion is just to put this in net/websockets. Since it's a header, > > excluding it won't break the build. > Currently, files in net/websockets are excluded in net/net.gyp if > enable_websockets != 1. I think it is difficult to re-include individual files. > And, now we have net/http/websocket_stream_base.cc in response to your comment. > So I would like to place websocket_stream_base.{h, cc} in net/http. Header files excluded from the build don't break compilation. Just cc files. Make WebSocketStreamBase pure virtual, and the problem is solved. I'm also fine with just disabling the WebSocket tests in this CL if WebSockets are not being built. https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... net/socket/client_socket_pool_manager.cc:92: request_url.SchemeIs("wss") || force_spdy_over_ssl; On 2013/05/30 04:44:32, yhirano wrote: > On 2013/05/28 21:22:37, mmenke wrote: > > Another issue for the CL to use the right HttpStreamFactory: If we're using > an > > HTTP proxy, we want to make sure we use it as a Connect proxy. I believe the > > current code will just connect to the HTTP proxy, and pass that on to the > > WebSocketStream. > > I'm not sure if I understand your comment correctly, but we use CONNECT when we > use a proxy server, as RFC6455 says in 4.1, _Proxy Usage_ > (http://tools.ietf.org/html/rfc6455#section-4.1). Is that resolve the problem? The issue is this code isn't using a CONNECT. Look at HttpProxyConnectJob::DoHttpProxyConnect and HttpProxyClientSocket::Connect. using_spdy_ will be false, as will params_->tunnel(). As a result, I believe this code won't issue a CONNECT request. Instead, it will determine a tunnel is not in use, and HttpProxyClientSocket::Connect will complete synchronously, without establishing the tunnel. https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... net/http/websocket_stream_base.h:22: // the object into a derived class depending on the object's type() property. Why will higher level code care about the type?
https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:40: class ScopedForceSpdySsl { I don't believe this class is needed. Looks like you're creating SPDY handshakes to me... If we aren't mocking enough of the code for those mock handshakes to take effect, then we shouldn't be using SSLSocketDataProviders at all, since they aren't doing anything. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:745: ssl_socket_data.protocol_negotiated = kProtoSPDY3; As-is, looks like this test is actually using SPDY, rather than HTTPS. I believe we can peek at the SPDY session pool to tell the difference between the two cases https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:750: session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_socket_data2); I don't believe we need two of these. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:866: ScopedForceSpdySsl use_spdy(false); I don't believe this is needed. We only upgrade to SPDY if the server indicates it supports it during SSL negotiation. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:875: ssl_socket_data.protocol_negotiated = kProtoSPDY3; This indicates SPDY 3 is being negotiated. I don't think we want that. See earlier comments. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:880: session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_socket_data2); Why are these two needed? https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:977: session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data2); Again, I don't think we need two of these.
In addition to your comments, I found a defect in my code: HttpStreamFactoryImpl::Job::CanUseExistingSpdySession returned false for wss:// connections. I fixed it. https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/117001/net/http/websocket_strea... net/http/websocket_stream_base.h:10: // you should not introduce dependency to net/websockets in this file. On 2013/05/30 19:27:12, mmenke wrote: > On 2013/05/30 04:44:32, yhirano wrote: > > On 2013/05/28 21:22:37, mmenke wrote: > > > My suggestion is just to put this in net/websockets. Since it's a header, > > > excluding it won't break the build. > > Currently, files in net/websockets are excluded in net/net.gyp if > > enable_websockets != 1. I think it is difficult to re-include individual > files. > > And, now we have net/http/websocket_stream_base.cc in response to your > comment. > > So I would like to place websocket_stream_base.{h, cc} in net/http. > > Header files excluded from the build don't break compilation. Just cc files. > Make WebSocketStreamBase pure virtual, and the problem is solved. > > I'm also fine with just disabling the WebSocket tests in this CL if WebSockets > are not being built. Done. https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/14813024/diff/117001/net/socket/client_socket... net/socket/client_socket_pool_manager.cc:92: request_url.SchemeIs("wss") || force_spdy_over_ssl; On 2013/05/30 19:27:12, mmenke wrote: > On 2013/05/30 04:44:32, yhirano wrote: > > On 2013/05/28 21:22:37, mmenke wrote: > > > Another issue for the CL to use the right HttpStreamFactory: If we're using > > an > > > HTTP proxy, we want to make sure we use it as a Connect proxy. I believe > the > > > current code will just connect to the HTTP proxy, and pass that on to the > > > WebSocketStream. > > > > I'm not sure if I understand your comment correctly, but we use CONNECT when > we > > use a proxy server, as RFC6455 says in 4.1, _Proxy Usage_ > > (http://tools.ietf.org/html/rfc6455#section-4.1). Is that resolve the problem? > > The issue is this code isn't using a CONNECT. Look at > HttpProxyConnectJob::DoHttpProxyConnect and HttpProxyClientSocket::Connect. > using_spdy_ will be false, as will params_->tunnel(). As a result, I believe > this code won't issue a CONNECT request. Instead, it will determine a tunnel is > not in use, and HttpProxyClientSocket::Connect will complete synchronously, > without establishing the tunnel. I see, I added InitSocketHandleForWebSocket which calls the helper function with force_tunnel = true in client_socket_pool_manager.h and use it if for_websocket_ is true. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:40: class ScopedForceSpdySsl { On 2013/05/30 19:46:14, mmenke wrote: > I don't believe this class is needed. Looks like you're creating SPDY > handshakes to me... If we aren't mocking enough of the code for those mock > handshakes to take effect, then we shouldn't be using SSLSocketDataProviders at > all, since they aren't doing anything. Done. In order to control was_spdy_negotiated in MockSSLClientSocket, I added some code to net/socket/socket_test_util.{h, cc}. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:745: ssl_socket_data.protocol_negotiated = kProtoSPDY3; On 2013/05/30 19:46:14, mmenke wrote: > As-is, looks like this test is actually using SPDY, rather than HTTPS. I > believe we can peek at the SPDY session pool to tell the difference between the > two cases Done. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:750: session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_socket_data2); On 2013/05/30 19:46:14, mmenke wrote: > I don't believe we need two of these. Done. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:866: ScopedForceSpdySsl use_spdy(false); On 2013/05/30 19:46:14, mmenke wrote: > I don't believe this is needed. We only upgrade to SPDY if the server indicates > it supports it during SSL negotiation. Done. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:875: ssl_socket_data.protocol_negotiated = kProtoSPDY3; On 2013/05/30 19:46:14, mmenke wrote: > This indicates SPDY 3 is being negotiated. I don't think we want that. See > earlier comments. Done. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:880: session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_socket_data2); On 2013/05/30 19:46:14, mmenke wrote: > Why are these two needed? Done. https://codereview.chromium.org/14813024/diff/128002/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:977: session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data2); On 2013/05/30 19:46:14, mmenke wrote: > Again, I don't think we need two of these. Done. https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... net/http/websocket_stream_base.h:22: // the object into a derived class depending on the object's type() property. On 2013/05/30 19:27:12, mmenke wrote: > Why will higher level code care about the type? WebSocketStreamBase is not in "natural" design. Most users (i.e. code in net/websockets) should downcast it to the appropriate derived type (e.g. WebSocketBasicStream). So I think I should explain how to downcast it.
https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... net/http/websocket_stream_base.h:22: // the object into a derived class depending on the object's type() property. On 2013/05/31 08:36:45, yhirano wrote: > On 2013/05/30 19:27:12, mmenke wrote: > > Why will higher level code care about the type? > > WebSocketStreamBase is not in "natural" design. Most users (i.e. code in > net/websockets) should downcast it to the appropriate derived type (e.g. > WebSocketBasicStream). So I think I should explain how to downcast it. How about a virtual AsWebSocketStream method that returns a net::WebSocketStream pointer? You can predeclare WebSocketStream so you don't need to include the definition, and then net::WebSocketStream can override it just to return "this". It should help to make it clear that WebSocketStream is the main interface definition, and avoid the need for down casts.
This is looking pretty good. I've mentioned a couple times that I think we should hook up the WebSocket pools in another CL before landing this one. Are you planning on doing this? Or do you want to land this first? Asking so I know whether you're expecting me to sign off on this before the other CL.
https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... net/http/websocket_stream_base.h:22: // the object into a derived class depending on the object's type() property. On 2013/05/31 16:46:42, Adam Rice wrote: > On 2013/05/31 08:36:45, yhirano wrote: > > On 2013/05/30 19:27:12, mmenke wrote: > > > Why will higher level code care about the type? > > > > WebSocketStreamBase is not in "natural" design. Most users (i.e. code in > > net/websockets) should downcast it to the appropriate derived type (e.g. > > WebSocketBasicStream). So I think I should explain how to downcast it. > > How about a virtual AsWebSocketStream method that returns a net::WebSocketStream > pointer? You can predeclare WebSocketStream so you don't need to include the > definition, and then net::WebSocketStream can override it just to return "this". > It should help to make it clear that WebSocketStream is the main interface > definition, and avoid the need for down casts. Cool! I will do that. Thank you!
On 2013/05/31 18:13:53, mmenke wrote: > This is looking pretty good. I've mentioned a couple times that I think we > should hook up the WebSocket pools in another CL before landing this one. Are > you planning on doing this? Or do you want to land this first? Asking so I > know whether you're expecting me to sign off on this before the other CL. I would like to land this CL first, and write the CL for WebSocket socket pool next.
https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... File net/http/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/128002/net/http/websocket_strea... net/http/websocket_stream_base.h:22: // the object into a derived class depending on the object's type() property. On 2013/05/31 18:40:06, yhirano wrote: > On 2013/05/31 16:46:42, Adam Rice wrote: > > On 2013/05/31 08:36:45, yhirano wrote: > > > On 2013/05/30 19:27:12, mmenke wrote: > > > > Why will higher level code care about the type? > > > > > > WebSocketStreamBase is not in "natural" design. Most users (i.e. code in > > > net/websockets) should downcast it to the appropriate derived type (e.g. > > > WebSocketBasicStream). So I think I should explain how to downcast it. > > > > How about a virtual AsWebSocketStream method that returns a > net::WebSocketStream > > pointer? You can predeclare WebSocketStream so you don't need to include the > > definition, and then net::WebSocketStream can override it just to return > "this". > > It should help to make it clear that WebSocketStream is the main interface > > definition, and avoid the need for down casts. > > Cool! I will do that. Thank you! Done.
Need to give the tests a final once over, but everything else I'm pretty much ready to sign off on. Sorry for being so slow on this. It's a complicated CL, and I've been busy this last week. https://codereview.chromium.org/14813024/diff/158001/net/http/http_network_se... File net/http/http_network_session.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_network_se... net/http/http_network_session.h:20: #include "net/ssl/ssl_client_auth_cache.h" While you're fixing the headers, mind adding "base/basictypes.h" for unint16 and base/memory/scoped_ptr.h? Thanks! https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory.h:197: // Request a websocket stream. Looks to me like websocket is only used 3 times in comments in net/, so I suggest on standardizing on WebSocket in comments (Except when referring to file names, or variables names websocket, of course). https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:148: const bool for_websockets_; I may have been wrong to suggest this - it's fine for now, but I had been thinking we'd be using different HttpSessions for websockets, rather than different socket pools in the same session. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:304: DCHECK(websocket_stream_); optional nit: Just to make parallel structure with HttpStreamFactoryImpl::Job::OnStreamReadyCallback clearer, may want to put "DCHECK(websocket_stream_);" first. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.h:130: // This callback function is called when a new spdy session is created. nit: SPDY (We aren't very consistent about this, but SPDY wins out). https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.h:121: // Orphan a job if appropriate. This comment is incorrect. It may orphan all jobs except |job|. It's also not at all clear what "appropriate" means. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.h:122: void Orphan(Job* job); This name is misleading, since it doesn't orphan |job|, but rather all other jobs. I suggest calling this "OnJobSucceeded" https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1354: bool MockSSLClientSocket::was_spdy_negotiated() const { This should be WasSpdyNegotiated, since it's not a simple function, and not inlined. https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1358: WasNpnNegotiated() && nit: Suggest putting this on the previous line (return just looks weird alone on a line). https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1360: GetNegotiatedProtocol() <= kProtoSPDYMaximumVersion; I find this confusing - since we have set_was_spdy_negotiated, one would assume this returns the last value it was called with. Think this is worth a comment. https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1363: bool MockSSLClientSocket::set_was_spdy_negotiated(bool negotiated) { It's my understanding that, according to the Google style guide, all functions named this would should be inlined. https://codereview.chromium.org/14813024/diff/158001/net/websockets/websocket... File net/websockets/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/158001/net/websockets/websocket... net/websockets/websocket_stream_base.h:10: // you should not introduce dependency to net/websockets in this file. nit: Don't use "you" in comments. Can use passive voice instead, like "... this file should not depend on net/websockets."
Also, could you update the description? ("RequestStream variant" =>
"RequestWebSocketStream")
Thank you, done! https://codereview.chromium.org/14813024/diff/158001/net/http/http_network_se... File net/http/http_network_session.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_network_se... net/http/http_network_session.h:20: #include "net/ssl/ssl_client_auth_cache.h" On 2013/06/03 21:10:18, mmenke wrote: > While you're fixing the headers, mind adding "base/basictypes.h" for unint16 and > base/memory/scoped_ptr.h? Thanks! Done. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory.h:197: // Request a websocket stream. On 2013/06/03 21:10:18, mmenke wrote: > Looks to me like websocket is only used 3 times in comments in net/, so I > suggest on standardizing on WebSocket in comments (Except when referring to file > names, or variables names websocket, of course). Done. I fixed files already modified in this CL (in fact, it is only this file). https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:304: DCHECK(websocket_stream_); On 2013/06/03 21:10:18, mmenke wrote: > optional nit: Just to make parallel structure with > HttpStreamFactoryImpl::Job::OnStreamReadyCallback clearer, may want to put > "DCHECK(websocket_stream_);" first. Done. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.h:130: // This callback function is called when a new spdy session is created. On 2013/06/03 21:10:18, mmenke wrote: > nit: SPDY (We aren't very consistent about this, but SPDY wins out). Done. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.h:121: // Orphan a job if appropriate. On 2013/06/03 21:10:18, mmenke wrote: > This comment is incorrect. It may orphan all jobs except |job|. It's also not > at all clear what "appropriate" means. I deleted this comment because the (new) name expresses what the function is well. https://codereview.chromium.org/14813024/diff/158001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.h:122: void Orphan(Job* job); On 2013/06/03 21:10:18, mmenke wrote: > This name is misleading, since it doesn't orphan |job|, but rather all other > jobs. I suggest calling this "OnJobSucceeded" Done. https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1354: bool MockSSLClientSocket::was_spdy_negotiated() const { On 2013/06/03 21:10:18, mmenke wrote: > This should be WasSpdyNegotiated, since it's not a simple function, and not > inlined. Since it is a derived function, I have no choice (unless I change the function name in the base class and it is beyond this CL). https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1358: WasNpnNegotiated() && On 2013/06/03 21:10:18, mmenke wrote: > nit: Suggest putting this on the previous line (return just looks weird alone > on a line). Done. https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1360: GetNegotiatedProtocol() <= kProtoSPDYMaximumVersion; On 2013/06/03 21:10:18, mmenke wrote: > I find this confusing - since we have set_was_spdy_negotiated, one would assume > this returns the last value it was called with. Think this is worth a comment. Done. https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1363: bool MockSSLClientSocket::set_was_spdy_negotiated(bool negotiated) { On 2013/06/03 21:10:18, mmenke wrote: > It's my understanding that, according to the Google style guide, all functions > named this would should be inlined. Moving the implementation into the header causes a warning: warning: [chromium-style] virtual methods with non-empty bodies shouldn't be declared inline. Again, I can't rename this function because this is a derived function. https://codereview.chromium.org/14813024/diff/158001/net/websockets/websocket... File net/websockets/websocket_stream_base.h (right): https://codereview.chromium.org/14813024/diff/158001/net/websockets/websocket... net/websockets/websocket_stream_base.h:10: // you should not introduce dependency to net/websockets in this file. On 2013/06/03 21:10:18, mmenke wrote: > nit: Don't use "you" in comments. Can use passive voice instead, like "... > this file should not depend on net/websockets." Done.
https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/14813024/diff/158001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1363: bool MockSSLClientSocket::set_was_spdy_negotiated(bool negotiated) { On 2013/06/04 02:20:13, yhirano wrote: > On 2013/06/03 21:10:18, mmenke wrote: > > It's my understanding that, according to the Google style guide, all functions > > named this would should be inlined. > Moving the implementation into the header causes a warning: > warning: [chromium-style] virtual methods with non-empty bodies shouldn't be > declared inline. > Again, I can't rename this function because this is a derived function. Sorry, didn't notice they were virtual. I may fix their naming in a future CL, but it's beyond the scope of this one. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (left): https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:464: // list. Why are you removing this comment? It looks correct to me. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:42: class WebSocketStream : public WebSocketStreamBase { This doesn't work. Once you have a WebSocketStream class for things other than tests, you'll have two net::WebSocketStreams, and linking will fail. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:48: explicit WebSocketStream(StreamType type) : type_(type) {} nit: Mind adding a couple blank lines here, for legibility? Just thinking one above and below the destructor. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:48: explicit WebSocketStream(StreamType type) : type_(type) {} Speaking of which...you should have a virtual destructor here. Suggest it for the other subclasses, too. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:49: virtual WebSocketStream* AsWebSocketStream() OVERRIDE {return this;} nit: "{ return this; }" https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:53: private: nit: Line break before private. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:59: class MockHttpStreamFactoryImplForPreconnect : public HttpStreamFactoryImpl { Mind adding a comment here, while you're renaming this? Something along the lines of "HttpStreamFactoryImpl subclass that can wait until a preconnect is complete." https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:181: explicit WebSocketSpdyStream(SpdySession* spdy_session): nit: Colon should go on next line (Also, if it goes on the same line as the close parenthesis, should have a space before it). https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:183: SpdySession* spdy_session() {return spdy_session_.get();} nit: { return spdy_session_.get(); } https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:193: ClientSocketHandle* connection() {return connection_.get();} nit: { return connection_.get(); } https://codereview.chromium.org/14813024/diff/185001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/14813024/diff/185001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1358: // Otherwise derive the value from other information. Still not following why this is needed - all tests that use this function use SSLConnectJobs to establish the connection, don't they? And SSLConnectJobs set this value on completion.
https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:42: class WebSocketStream : public WebSocketStreamBase { On 2013/06/05 00:01:20, mmenke wrote: > This doesn't work. Once you have a WebSocketStream class for things other than > tests, you'll have two net::WebSocketStreams, and linking will fail. Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:48: explicit WebSocketStream(StreamType type) : type_(type) {} On 2013/06/05 00:01:20, mmenke wrote: > Speaking of which...you should have a virtual destructor here. Suggest it for > the other subclasses, too. Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:48: explicit WebSocketStream(StreamType type) : type_(type) {} On 2013/06/05 00:01:20, mmenke wrote: > nit: Mind adding a couple blank lines here, for legibility? Just thinking one > above and below the destructor. Done (constructor, you mean?). https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:49: virtual WebSocketStream* AsWebSocketStream() OVERRIDE {return this;} On 2013/06/05 00:01:20, mmenke wrote: > nit: "{ return this; }" Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:53: private: On 2013/06/05 00:01:20, mmenke wrote: > nit: Line break before private. Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:59: class MockHttpStreamFactoryImplForPreconnect : public HttpStreamFactoryImpl { On 2013/06/05 00:01:20, mmenke wrote: > Mind adding a comment here, while you're renaming this? Something along the > lines of "HttpStreamFactoryImpl subclass that can wait until a preconnect is > complete." Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:181: explicit WebSocketSpdyStream(SpdySession* spdy_session): On 2013/06/05 00:01:20, mmenke wrote: > nit: Colon should go on next line (Also, if it goes on the same line as the > close parenthesis, should have a space before it). Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:183: SpdySession* spdy_session() {return spdy_session_.get();} On 2013/06/05 00:01:20, mmenke wrote: > nit: { return spdy_session_.get(); } Done. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:193: ClientSocketHandle* connection() {return connection_.get();} On 2013/06/05 00:01:20, mmenke wrote: > nit: { return connection_.get(); } Done. https://codereview.chromium.org/14813024/diff/185001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/14813024/diff/185001/net/socket/socket_test_u... net/socket/socket_test_util.cc:1358: // Otherwise derive the value from other information. On 2013/06/05 00:01:20, mmenke wrote: > Still not following why this is needed - all tests that use this function use > SSLConnectJobs to establish the connection, don't they? And SSLConnectJobs set > this value on completion. You are right. Done.
Just one issue that's significant. https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/185001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:48: explicit WebSocketStream(StreamType type) : type_(type) {} On 2013/06/06 14:06:43, yhirano wrote: > On 2013/06/05 00:01:20, mmenke wrote: > > nit: Mind adding a couple blank lines here, for legibility? Just thinking > one > > above and below the destructor. > > Done (constructor, you mean?). Actually, I meant the destructor that I suggested you add in the above comment. :) https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:21: #include "net/socket/client_socket_pool_manager.h" nit: I don't believe this is used. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:87: net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_HTTP_STREAM_JOB)), Suggestion for another CL: If this is for a WebSocketStream, we could use different NetLog source type. In the future, this could make it easier to debug WebSocket issues. Suggest another CL because net-internals code would need to have a little extra code, too, and think it's easier just to worry about it in another CL. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:130: stream_->Close(true /* not reusable */); I'm assuming that WebSocketStreams do not need similar treatment, since they'll never return anything to the socket pool. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:519: } else { optional nit: Just to keep this structure parallel with the above code, may want to add "DCHECK(stream_.get());" here. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); If we're orphaned before this point, request_ will be NULL, and we're in trouble here. Seems like there are a couple: 1) Have the HttpStreamFactory have a single stream factory pointer, and just use that for all jobs. 2) Have the Jobs keep track of their HttpStreamFactory pointers themselves. 3) Fail WebSocket jobs once they're orphaned. This may be the way to go. I'm not sure what the expected lifetime of a WebSocketStream is going to be, or what the odds are of destroying one and then re-creating one for the same server. May be other ways to bypass the orphan logic for WebSocket jobs. I think we should also have a unit test for this case. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.h:132: void OnWebSocketStreamReadyCallback(); nit: Should match order in the cc file (Also makes more sense to put this next to OnStreamReadyCallback). https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.cc:333: websocket_stream_factory_; nit: This should be below the "if (factory->for_websockets_) {" line. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:204: virtual ~WebSocketBasicStream() {} Suggest you call connection_.socket()->Disconnect() (Or whatever it's called), to more accurately mirror what a real WebSocketBasicStream should do. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:213: public: Should have a virtual destructor, per Google style guide. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:738: EXPECT_TRUE(NULL != waiter.stream()); ASSERT_TRUE (Same for all the other NULL checks like this that would subsequently result in crashes if skipped). https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:781: EXPECT_TRUE(NULL != waiter.stream()); nit: Should use consistent order for these two tests, just for fast visual comparison. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:866: static_cast<MockWebSocketStream*>(waiter.websocket_stream()) Suggest having the waiter do the cast to MockWebSocketStream, so you can avoid this awkward line breaking. Or could just reduce the indent of the static_cast lines to use a 4-space indent instead of aligning to the parentheses, so it all fits on one line.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:21: #include "net/socket/client_socket_pool_manager.h" On 2013/06/10 20:26:03, mmenke wrote: > nit: I don't believe this is used. Done. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:87: net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_HTTP_STREAM_JOB)), On 2013/06/10 20:26:03, mmenke wrote: > Suggestion for another CL: If this is for a WebSocketStream, we could use > different NetLog source type. In the future, this could make it easier to debug > WebSocket issues. > > Suggest another CL because net-internals code would need to have a little extra > code, too, and think it's easier just to worry about it in another CL. Filed a bug. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:130: stream_->Close(true /* not reusable */); On 2013/06/10 20:26:03, mmenke wrote: > I'm assuming that WebSocketStreams do not need similar treatment, since they'll > never return anything to the socket pool. I think so. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:519: } else { On 2013/06/10 20:26:03, mmenke wrote: > optional nit: Just to keep this structure parallel with the above code, may > want to add "DCHECK(stream_.get());" here. Done. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/10 20:26:03, mmenke wrote: > If we're orphaned before this point, request_ will be NULL, and we're in trouble > here. Seems like there are a couple: > > 1) Have the HttpStreamFactory have a single stream factory pointer, and just > use that for all jobs. > 2) Have the Jobs keep track of their HttpStreamFactory pointers themselves. > 3) Fail WebSocket jobs once they're orphaned. This may be the way to go. I'm > not sure what the expected lifetime of a WebSocketStream is going to be, or what > the odds are of destroying one and then re-creating one for the same server. > May be other ways to bypass the orphan logic for WebSocket jobs. > > I think we should also have a unit test for this case. I chose the third way. > I think we should also have a unit test for this case. I tried, but I could not create the situation. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.h:132: void OnWebSocketStreamReadyCallback(); On 2013/06/10 20:26:03, mmenke wrote: > nit: Should match order in the cc file (Also makes more sense to put this next > to OnStreamReadyCallback). Done. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.cc:333: websocket_stream_factory_; On 2013/06/10 20:26:03, mmenke wrote: > nit: This should be below the "if (factory->for_websockets_) {" line. I deleted this variable. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:204: virtual ~WebSocketBasicStream() {} On 2013/06/10 20:26:03, mmenke wrote: > Suggest you call connection_.socket()->Disconnect() (Or whatever it's called), > to more accurately mirror what a real WebSocketBasicStream should do. Done, but I don't understand why this is necessary. Can you tell me? https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:213: public: On 2013/06/10 20:26:03, mmenke wrote: > Should have a virtual destructor, per Google style guide. Done. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:738: EXPECT_TRUE(NULL != waiter.stream()); On 2013/06/10 20:26:03, mmenke wrote: > ASSERT_TRUE (Same for all the other NULL checks like this that would > subsequently result in crashes if skipped). Done. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:781: EXPECT_TRUE(NULL != waiter.stream()); On 2013/06/10 20:26:03, mmenke wrote: > nit: Should use consistent order for these two tests, just for fast visual > comparison. Done. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:866: static_cast<MockWebSocketStream*>(waiter.websocket_stream()) On 2013/06/10 20:26:03, mmenke wrote: > Suggest having the waiter do the cast to MockWebSocketStream, so you can avoid > this awkward line breaking. Or could just reduce the indent of the static_cast > lines to use a 4-space indent instead of aligning to the parentheses, so it all > fits on one line. Done.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/11 11:13:16, yhirano wrote: > On 2013/06/10 20:26:03, mmenke wrote: > > If we're orphaned before this point, request_ will be NULL, and we're in > trouble > > here. Seems like there are a couple: > > > > 1) Have the HttpStreamFactory have a single stream factory pointer, and just > > use that for all jobs. > > 2) Have the Jobs keep track of their HttpStreamFactory pointers themselves. > > 3) Fail WebSocket jobs once they're orphaned. This may be the way to go. > I'm > > not sure what the expected lifetime of a WebSocketStream is going to be, or > what > > the odds are of destroying one and then re-creating one for the same server. > > May be other ways to bypass the orphan logic for WebSocket jobs. > > > > I think we should also have a unit test for this case. > I chose the third way. > > > I think we should also have a unit test for this case. > I tried, but I could not create the situation. Just deleting the waiter in the unit tests before we get a stream, and then waiting until idle and making sure the pool has no active/idle sockets doesn't work? I'd really like a test for this case if we can manage one - think it'll be an obscure enough case that it will easily be overlooked in future refactors. I'll investigate myself tomorrow, if you're still unable to trigger the codepath. Think we can land the CL after that, though I want to do a final once over. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:204: virtual ~WebSocketBasicStream() {} On 2013/06/11 11:13:16, yhirano wrote: > On 2013/06/10 20:26:03, mmenke wrote: > > Suggest you call connection_.socket()->Disconnect() (Or whatever it's > called), > > to more accurately mirror what a real WebSocketBasicStream should do. > > Done, but I don't understand why this is necessary. Can you tell me? When a ClientSocketHandle is deleted, by default, it returns the socket to the SocketPool. The only way to avoid this is to explicity close the socket before deleting the handle.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/12 02:27:05, mmenke wrote: > On 2013/06/11 11:13:16, yhirano wrote: > > On 2013/06/10 20:26:03, mmenke wrote: > > > If we're orphaned before this point, request_ will be NULL, and we're in > > trouble > > > here. Seems like there are a couple: > > > > > > 1) Have the HttpStreamFactory have a single stream factory pointer, and > just > > > use that for all jobs. > > > 2) Have the Jobs keep track of their HttpStreamFactory pointers themselves. > > > 3) Fail WebSocket jobs once they're orphaned. This may be the way to go. > > I'm > > > not sure what the expected lifetime of a WebSocketStream is going to be, or > > what > > > the odds are of destroying one and then re-creating one for the same server. > > > > May be other ways to bypass the orphan logic for WebSocket jobs. > > > > > > I think we should also have a unit test for this case. > > I chose the third way. > > > > > I think we should also have a unit test for this case. > > I tried, but I could not create the situation. > > Just deleting the waiter in the unit tests before we get a stream, and then > waiting until idle and making sure the pool has no active/idle sockets doesn't > work? > > I'd really like a test for this case if we can manage one - think it'll be an > obscure enough case that it will easily be overlooked in future refactors. > > I'll investigate myself tomorrow, if you're still unable to trigger the > codepath. Think we can land the CL after that, though I want to do a final once > over. You may need to create mock socket data that connects asynchronously, though since we call Job::OnStreamReadyCallback asynchronously, I don't think this is necessary.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/12 02:31:13, mmenke wrote: > On 2013/06/12 02:27:05, mmenke wrote: > > On 2013/06/11 11:13:16, yhirano wrote: > > > On 2013/06/10 20:26:03, mmenke wrote: > > > > If we're orphaned before this point, request_ will be NULL, and we're in > > > trouble > > > > here. Seems like there are a couple: > > > > > > > > 1) Have the HttpStreamFactory have a single stream factory pointer, and > > just > > > > use that for all jobs. > > > > 2) Have the Jobs keep track of their HttpStreamFactory pointers > themselves. > > > > 3) Fail WebSocket jobs once they're orphaned. This may be the way to go. > > > > I'm > > > > not sure what the expected lifetime of a WebSocketStream is going to be, > or > > > what > > > > the odds are of destroying one and then re-creating one for the same > server. > > > > > > May be other ways to bypass the orphan logic for WebSocket jobs. > > > > > > > > I think we should also have a unit test for this case. > > > I chose the third way. > > > > > > > I think we should also have a unit test for this case. > > > I tried, but I could not create the situation. > > > > Just deleting the waiter in the unit tests before we get a stream, and then > > waiting until idle and making sure the pool has no active/idle sockets doesn't > > work? > > > > I'd really like a test for this case if we can manage one - think it'll be an > > obscure enough case that it will easily be overlooked in future refactors. > > > > I'll investigate myself tomorrow, if you're still unable to trigger the > > codepath. Think we can land the CL after that, though I want to do a final > once > > over. > > You may need to create mock socket data that connects asynchronously, though > since we call Job::OnStreamReadyCallback asynchronously, I don't think this is > necessary. Actually, I think we should make sure there is a pending connection, then delete the request, then make sure there are no pending connections, then run until idle, and check again.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); > Just deleting the waiter in the unit tests before we get a stream, and then waiting until idle and making sure the pool has no active/idle sockets doesn't work? Neither deleting waiter nor request call HttpStreamFactoryImpl::OrphanJob nor HttpStreamFactoryImpl::Job::Orphan (I believe you meant request, not waiter). I think there should be an OrphanJobs call in HttpStreamFactoryImpl::Request::~Request, but I'm not confident of that. https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:204: virtual ~WebSocketBasicStream() {} On 2013/06/12 02:27:05, mmenke wrote: > On 2013/06/11 11:13:16, yhirano wrote: > > On 2013/06/10 20:26:03, mmenke wrote: > > > Suggest you call connection_.socket()->Disconnect() (Or whatever it's > > called), > > > to more accurately mirror what a real WebSocketBasicStream should do. > > > > Done, but I don't understand why this is necessary. Can you tell me? > > When a ClientSocketHandle is deleted, by default, it returns the socket to the > SocketPool. The only way to avoid this is to explicity close the socket before > deleting the handle. Thank you, I understand.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/12 06:44:56, yhirano wrote: > > Just deleting the waiter in the unit tests before we get a stream, and then > waiting until idle and making sure the pool has no active/idle sockets doesn't > work? > > Neither deleting waiter nor request call HttpStreamFactoryImpl::OrphanJob nor > HttpStreamFactoryImpl::Job::Orphan (I believe you meant request, not waiter). > I think there should be an OrphanJobs call in > HttpStreamFactoryImpl::Request::~Request, but I'm not confident of that. You're right - all calls to OrphanJobs go through OrphanJobsExcept, so we need to have multiple jobs for a request to reach that code. Requests are added in HttpStreamFactoryImpl::RequestStream, when we get an alternate scheme. Deleting is also not enough to trigger orphaning other jobs. I still feel we should probably have a test for this, but I'm going to have to do some digging to figure out how to do that.
https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/12 18:21:33, mmenke wrote: > On 2013/06/12 06:44:56, yhirano wrote: > > > Just deleting the waiter in the unit tests before we get a stream, and then > > waiting until idle and making sure the pool has no active/idle sockets doesn't > > work? > > > > Neither deleting waiter nor request call HttpStreamFactoryImpl::OrphanJob nor > > HttpStreamFactoryImpl::Job::Orphan (I believe you meant request, not waiter). > > I think there should be an OrphanJobs call in > > HttpStreamFactoryImpl::Request::~Request, but I'm not confident of that. > > You're right - all calls to OrphanJobs go through OrphanJobsExcept, so we need > to have multiple jobs for a request to reach that code. Requests are added in > HttpStreamFactoryImpl::RequestStream, when we get an alternate scheme. Deleting > is also not enough to trigger orphaning other jobs. > > I still feel we should probably have a test for this, but I'm going to have to > do some digging to figure out how to do that. Done. To control the timing of socket connection, I added net::AsyncSocket::OnConnectComplete and changed the implementation of MockTcpClientSocket::Connect a bit. I am examining if the change breaks other tests by using trybots.
On 2013/06/13 14:20:38, yhirano wrote: > https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... > net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && > request_->websocket_stream_factory()); > On 2013/06/12 18:21:33, mmenke wrote: > > On 2013/06/12 06:44:56, yhirano wrote: > > > > Just deleting the waiter in the unit tests before we get a stream, and > then > > > waiting until idle and making sure the pool has no active/idle sockets > doesn't > > > work? > > > > > > Neither deleting waiter nor request call HttpStreamFactoryImpl::OrphanJob > nor > > > HttpStreamFactoryImpl::Job::Orphan (I believe you meant request, not > waiter). > > > I think there should be an OrphanJobs call in > > > HttpStreamFactoryImpl::Request::~Request, but I'm not confident of that. > > > > You're right - all calls to OrphanJobs go through OrphanJobsExcept, so we need > > to have multiple jobs for a request to reach that code. Requests are added in > > HttpStreamFactoryImpl::RequestStream, when we get an alternate scheme. > Deleting > > is also not enough to trigger orphaning other jobs. > > > > I still feel we should probably have a test for this, but I'm going to have to > > do some digging to figure out how to do that. > > Done. > To control the timing of socket connection, I added > net::AsyncSocket::OnConnectComplete and changed the implementation of > MockTcpClientSocket::Connect a bit. > I am examining if the change breaks other tests by using trybots. Sorry, the CL causes a build error. I will fix it tomorrow. mmenke@, can you take a look at it, though?
On 2013/06/13 15:35:38, yhirano wrote: > On 2013/06/13 14:20:38, yhirano wrote: > > > https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... > > File net/http/http_stream_factory_impl_job.cc (right): > > > > > https://codereview.chromium.org/14813024/diff/209001/net/http/http_stream_fac... > > net/http/http_stream_factory_impl_job.cc:1028: DCHECK(request_ && > > request_->websocket_stream_factory()); > > On 2013/06/12 18:21:33, mmenke wrote: > > > On 2013/06/12 06:44:56, yhirano wrote: > > > > > Just deleting the waiter in the unit tests before we get a stream, and > > then > > > > waiting until idle and making sure the pool has no active/idle sockets > > doesn't > > > > work? > > > > > > > > Neither deleting waiter nor request call HttpStreamFactoryImpl::OrphanJob > > nor > > > > HttpStreamFactoryImpl::Job::Orphan (I believe you meant request, not > > waiter). > > > > I think there should be an OrphanJobs call in > > > > HttpStreamFactoryImpl::Request::~Request, but I'm not confident of that. > > > > > > You're right - all calls to OrphanJobs go through OrphanJobsExcept, so we > need > > > to have multiple jobs for a request to reach that code. Requests are added > in > > > HttpStreamFactoryImpl::RequestStream, when we get an alternate scheme. > > Deleting > > > is also not enough to trigger orphaning other jobs. > > > > > > I still feel we should probably have a test for this, but I'm going to have > to > > > do some digging to figure out how to do that. > > > > Done. > > To control the timing of socket connection, I added > > net::AsyncSocket::OnConnectComplete and changed the implementation of > > MockTcpClientSocket::Connect a bit. > > I am examining if the change breaks other tests by using trybots. > > Sorry, the CL causes a build error. I will fix it tomorrow. > mmenke@, can you take a look at it, though? Will do.
https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:68: class Request; nit: Is this used in the unit tests? https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:73: virtual void OnOrphanedJobComplete(const Job* job); If you follow my other comment, these may not be needed - can just add a function to get the number of orphaned jobs, and verify there aren't any. https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1035: return ERR_FAILED; I think the right thing to do, in order to cancel orphaned jobs, is to just call stream_factory_->OnOrphanedJobComplete in Job::Orphan, for websocket requests. Code should go after the blocking_job_ abort case. Then you can get rid of these ERR_FAILEDs, and add a DCHECK that the job is not orphaned, both before the two ERR_FAILED calls, and OnWebSocketStreamReadyCallback. https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1138: ASSERT_EQ(1, http_websocket_stream_factory->counter()); Aren't we cancelling orphaned jobs?
https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:68: class Request; On 2013/06/13 17:59:56, mmenke wrote: > nit: Is this used in the unit tests? Reverted. https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl.h:73: virtual void OnOrphanedJobComplete(const Job* job); On 2013/06/13 17:59:56, mmenke wrote: > If you follow my other comment, these may not be needed - can just add a > function to get the number of orphaned jobs, and verify there aren't any. Done. https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1035: return ERR_FAILED; On 2013/06/13 17:59:56, mmenke wrote: > I think the right thing to do, in order to cancel orphaned jobs, is to just call > stream_factory_->OnOrphanedJobComplete in Job::Orphan, for websocket requests. > Code should go after the blocking_job_ abort case. > > Then you can get rid of these ERR_FAILEDs, and add a DCHECK that the job is not > orphaned, both before the two ERR_FAILED calls, and > OnWebSocketStreamReadyCallback. Done. https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/249001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1138: ASSERT_EQ(1, http_websocket_stream_factory->counter()); On 2013/06/13 17:59:56, mmenke wrote: > Aren't we cancelling orphaned jobs? Done.
The end is in sight! https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:93: DCHECK(for_websockets_); Maybe a DCHECK(factory) as well? https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:112: new Request(request_info.url, optional nit: May be able to move the "new Request" onto the previous line. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1105: DCHECK(request_ && request_->websocket_stream_factory()); nit: Split into two DCHECKs, so it's clear which part failed? Don't think short-circuiting the second one with the first to prevent crashing is going to help a whole lot here. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.cc:100: void HttpStreamFactoryImpl::Request::OnJobSucceeded(Job* job) { nit: Was great to keep this here for easy review, but should be moved to the same order it's declared in the header. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.cc:374: if (factory_->for_websockets_) { I think this should go in Job::Orphan - we already do something similar there if the request hasn't started yet. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:814: TEST(HttpStreamFactoryTest, RequestHttpStreamOverProxy) { There's no WebSocket version of this one? https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1113: session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL))); Shouldn't the socket have been deleted by this point?
https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:93: DCHECK(for_websockets_); On 2013/06/17 19:55:01, mmenke wrote: > Maybe a DCHECK(factory) as well? Done. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl.cc:112: new Request(request_info.url, On 2013/06/17 19:55:01, mmenke wrote: > optional nit: May be able to move the "new Request" onto the previous line. Done. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:1105: DCHECK(request_ && request_->websocket_stream_factory()); On 2013/06/17 19:55:01, mmenke wrote: > nit: Split into two DCHECKs, so it's clear which part failed? Don't think > short-circuiting the second one with the first to prevent crashing is going to > help a whole lot here. Done. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.cc:100: void HttpStreamFactoryImpl::Request::OnJobSucceeded(Job* job) { On 2013/06/17 19:55:01, mmenke wrote: > nit: Was great to keep this here for easy review, but should be moved to the > same order it's declared in the header. Done. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_request.cc:374: if (factory_->for_websockets_) { On 2013/06/17 19:55:01, mmenke wrote: > I think this should go in Job::Orphan - we already do something similar there if > the request hasn't started yet. Done. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:814: TEST(HttpStreamFactoryTest, RequestHttpStreamOverProxy) { On 2013/06/17 19:55:01, mmenke wrote: > There's no WebSocket version of this one? Done. https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1113: session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL))); On 2013/06/17 19:55:01, mmenke wrote: > Shouldn't the socket have been deleted by this point? I think deleted (pooled) socket is counted. I think pooling the socket is not a problem here, is it?
https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1113: session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL))); On 2013/06/18 08:34:14, yhirano wrote: > On 2013/06/17 19:55:01, mmenke wrote: > > Shouldn't the socket have been deleted by this point? > > I think deleted (pooled) socket is counted. > I think pooling the socket is not a problem here, is it? I'm not sure, which is the problem.
https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1113: session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL))); On 2013/06/18 20:21:45, mmenke wrote: > On 2013/06/18 08:34:14, yhirano wrote: > > On 2013/06/17 19:55:01, mmenke wrote: > > > Shouldn't the socket have been deleted by this point? > > > > I think deleted (pooled) socket is counted. > > I think pooling the socket is not a problem here, is it? > > I'm not sure, which is the problem. OK, I added the Disconnect code in http_stream_factory_impl_job.cc. Nevertheless, this expectation is correct because the job is canceled before the ClientSocketHandle binds the Socket. Since there is not data transmission in the socket, there is no problem to pool it for reuse.
On 2013/06/19 11:18:37, yhirano wrote: > https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... > File net/http/http_stream_factory_impl_unittest.cc (right): > > https://codereview.chromium.org/14813024/diff/257001/net/http/http_stream_fac... > net/http/http_stream_factory_impl_unittest.cc:1113: > session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL))); > On 2013/06/18 20:21:45, mmenke wrote: > > On 2013/06/18 08:34:14, yhirano wrote: > > > On 2013/06/17 19:55:01, mmenke wrote: > > > > Shouldn't the socket have been deleted by this point? > > > > > > I think deleted (pooled) socket is counted. > > > I think pooling the socket is not a problem here, is it? > > > > I'm not sure, which is the problem. > > OK, I added the Disconnect code in http_stream_factory_impl_job.cc. > Nevertheless, this expectation is correct because the job is canceled before the > ClientSocketHandle binds the Socket. Since there is not data transmission in the > socket, there is no problem to pool it for reuse. No data transmitted? What about proxies? Admittedly, we should be doing the same setup for SPDY and non-SPDY in the WebSocket case. What about sockets that have yet to go through DoInitConnectionComplete as opposed to those that have called DoCreateStreamComplete, and are just waiting for the async callback? It may all turn out fine, but the fact is there's a whole bunch of SPDY/QUIC/SSL related code there that I'm not familiar with, and understanding it all well enough to figure out if we're fine, both in the cases we set up a socket for SPDY/non-SPDY (For all proxy types), and then we properly reuse it is non-trivial.
LGTM, modulo two comments. Also, if you prefer the old orphaned behavior, if we can get rid of the WebSocketFactory pointer in the future, we can revert to it (Or we could just make the Job tack the factory pointer. That does make for weird lifetime issues, though, so if we do that, it should probably be moved in the HttpNetworkSession). https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:315: if (IsOrphaned()) { This should be: "DCHECK(!IsOrphaned());", without the branch, right, since we close orphaned jobs? https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1159: EXPECT_EQ(2, GetSocketPoolGroupCount( This should be 1 now, right?
https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... net/http/http_stream_factory_impl_unittest.cc:1159: EXPECT_EQ(2, GetSocketPoolGroupCount( On 2013/06/19 14:30:28, mmenke wrote: > This should be 1 now, right? I'm not sure what you meant, but it is 2 for PS39 and otherwise the test fails.
On 2013/06/19 14:37:13, yhirano wrote: > https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... > File net/http/http_stream_factory_impl_unittest.cc (right): > > https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... > net/http/http_stream_factory_impl_unittest.cc:1159: EXPECT_EQ(2, > GetSocketPoolGroupCount( > On 2013/06/19 14:30:28, mmenke wrote: > > This should be 1 now, right? > I'm not sure what you meant, but it is 2 for PS39 and otherwise the test fails. If we disconnect the socket, it should not be returned to the pool. Want to figure out what's going on there.
> If we disconnect the socket, it should not be returned to the pool. Want to > figure out what's going on there. connection_->socket() is null at HttpStreamFactoryImpl::Job::Orphan and so it is not disconnected.
On 2013/06/19 14:45:32, yhirano wrote: > > If we disconnect the socket, it should not be returned to the pool. Want to > > figure out what's going on there. > connection_->socket() is null at HttpStreamFactoryImpl::Job::Orphan and so it is > not disconnected. My concern here continues to be the fact that SPDY sockets come from the normal, non-SPDY pool, and I currently have no clue how they're differentiated, so returning a socket set up for SPDY to the pool could be bad. This may be a complete non-issue; I just am not currently familiar enough with the SPDY setup code to be sure. I'll dig through it today. Sorry for delaying this CL, we just don't do a great job of differentiating socket types, and I want to be sure we're not introducing some subtle, difficult-to-reproduce bug.
I'm sorry, I misunderstood. GetSocketPoolGroupCount returns the number of distinct groups in a socket pool, just as its name. So, GetSocketPoolGroupCount(pool) == 2 means there is two groups in the pool. It does not mean there is two sockets in the pool. We want to check that an alternative request is actually made (otherwise this test tests nothing!) and EXPECTｓ about GetSocketPoolGroupCount checks it. https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... net/http/http_stream_factory_impl_job.cc:315: if (IsOrphaned()) { On 2013/06/19 14:30:28, mmenke wrote: > This should be: > > "DCHECK(!IsOrphaned());", without the branch, right, since we close orphaned > jobs? Done.
On 2013/06/20 05:01:43, yhirano wrote: > I'm sorry, I misunderstood. > GetSocketPoolGroupCount returns the number of distinct groups in a socket pool, > just as its name. > So, GetSocketPoolGroupCount(pool) == 2 means there is two groups in the pool. It > does not mean there is two sockets in the pool. > We want to check that an alternative request is actually made (otherwise this > test tests nothing!) and EXPECTｓ about GetSocketPoolGroupCount checks it. > > https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/14813024/diff/287001/net/http/http_stream_fac... > net/http/http_stream_factory_impl_job.cc:315: if (IsOrphaned()) { > On 2013/06/19 14:30:28, mmenke wrote: > > This should be: > > > > "DCHECK(!IsOrphaned());", without the branch, right, since we close orphaned > > jobs? > > Done. This CL LGTM (Really, this time!) - and we don't need the disconnects you added in the last set, either.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/14813024/297001
Message was sent while issue was closed.
Change committed as 207735 |
