Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296053012:
Replace StreamListenSocket with StreamSocket in HttpServer. (Closed)
|
Created:
6 years, 7 months ago by byungchul Modified:
6 years, 4 months ago Reviewers:
mnaganov (inactive), Ryan Sleevi, vkuzkokov, gunsch, Yaron, pfeldman, darin (slow to review), mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, vsevik, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yurys, abarth-chromium, Aaron Boodman, paulirish+reviews_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, devtools-reviews_chromium.org, stgao, aandrey+blink_chromium.org, ben+mojo_chromium.org, darin (slow to review), gunsch, mnaganov (inactive) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionReplace StreamListenSocket with StreamSocket in HttpServer.
1) HttpServer gets ServerSocket instead of StreamListenSocket.
2) HttpConnection is just a container for socket, websocket, and pending read/write buffers.
3) HttpServer handles data buffering and asynchronous read/write.
4) HttpConnection has limit in data buffering, up to 1Mbytes by default.
5) For devtools, send buffer limit is 100Mbytes.
6) Unittests for buffer handling in HttpConnection.
BUG=371906
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291447
Patch Set 1 #
Total comments: 70
Patch Set 2 : Rebased #Patch Set 3 : Apply comments. Move async read/write to HttpServer #Patch Set 4 : Use base::StringPiece in some places not to copy data. #
Total comments: 35
Patch Set 5 : Rebased #
Total comments: 2
Patch Set 6 : Unitests and draft implementation of unix domain server socket. #Patch Set 7 : Unittests for unix domain server/client socket #Patch Set 8 : Fix compilation errors. #Patch Set 9 : Rebased and fix content_unittests #Patch Set 10 : Fix compilation error on win #Patch Set 11 : Don't export HttpServer which is built in a static lib #
Total comments: 42
Patch Set 12 : Rebased #Patch Set 13 : Fix compilation error #
Total comments: 16
Patch Set 14 : Rebased #Patch Set 15 : Fixed compilation erros. #Patch Set 16 : Fixed compilation errors. #Patch Set 17 : Fixed compilation errors in unittests. #Patch Set 18 : Applied comments #Patch Set 19 : Redo changes reverted with unknown reason. #
Total comments: 38
Patch Set 20 : Rebased #Patch Set 21 : Addressed comments. #
Total comments: 36
Patch Set 22 : Addressed comments. #Patch Set 23 : Fixed unittest failures. #
Total comments: 8
Patch Set 24 : Rebased, and addressed comments. #
Total comments: 4
Patch Set 25 : Addressed comments. #Patch Set 26 : Added a comment on HttpServer::HasClosedConnection(). #Patch Set 27 : Fixed unittest errors. #
Total comments: 3
Patch Set 28 : Rebased and fixed chromecast part. #Patch Set 29 : Disable a checking in HttpServerTest.MultipleRequestsOnSameConnection #Messages
Total messages: 73 (0 generated)
Please review this CL which is not yet completed, but want to get some review before going further. Main changes are in http_connection.cc/.h. Others are changed accordingly.
Only started my review, but need to switch machines, so uploading these. https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/ser... File chrome/test/chromedriver/server/chromedriver_server.cc (right): https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/ser... chrome/test/chromedriver/server/chromedriver_server.cc:58: if (!server_) { style nit: we don't put {} around single line blocks. https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... File content/browser/devtools/devtools_browser_target.cc (right): https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... content/browser/devtools/devtools_browser_target.cc:139: if (!http_server_) if (!http_server_->get()) https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... File content/browser/devtools/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... content/browser/devtools/devtools_http_handler_impl.cc:121: base::WeakPtr<net::HttpServer> server_; It was not refcounting it, so you should be fine with leaving it as raw.
Also note that it is typical for remote debugging payloads to be 10-100Mb and we run it in the threads that can block.
On 2014/05/22 06:03:54, pfeldman wrote: > Also note that it is typical for remote debugging payloads to be 10-100Mb and we > run it in the threads that can block. So sorry for not getting to this today, had an interview and a bunch of meetings. I'll take a look tomorrow.
A lot of these are just nits, still need to spend more time looking at it at a high level. https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/net... File chrome/test/chromedriver/net/test_http_server.cc (right): https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/net... chrome/test/chromedriver/net/test_http_server.cc:133: EXPECT_NE(NULL, server_.get()); If this fails, we'll crash 2 lines down. Suggest: *success = false; ASSERT_NE(...) https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:125: pending_data_.push(data); Can't we just take IOBuffers instead, to avoid this copy? https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:157: DCHECK_GT(pending_data_.front().size(), consumed); This class may be may effort than it's worth, as-is, it works it fairly non-obvious ways. One option would just be to have the HttpConnection directly manage the item at the top of the queue, and have this class manage everything else. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:157: DCHECK_GT(pending_data_.front().size(), consumed); Actually...Why don't we do something simpler, like just not supporting pipelining, and only having one send call until the previous one completes? https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:175: void HttpConnection::DoReadLoop(int rv) { Taking in an "rv" that's always OK seems a little odd. Should either get rid of the parameter, or restructure the loop so it takes the rv of the last read as input (The latter is a little complicated, because on the first call, it will be 0, which is the same value we get on EOF). https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:191: AsWeakPtr())); We don't need weak pointers here - we own the socket, and it won't call us after we delete it. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:231: Send(std::string(bytes, len)); This copy really shouldn't be needed. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:238: void HttpConnection::DoWriteLoop(int rv) { Again, rv is always "OK", so doesn't seem like it should be needed. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:243: AsWeakPtr())); WeakPtr not needed. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:267: void HttpConnection::Close() { This function can end up being called twice: Once on read error, once on write error, which could result in calling into the delegate twice, which seems like a potential problem. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:272: base::Bind(&HttpConnection::CloseInNextRunLoop, AsWeakPtr())); So we always call back Delegate::DidClose asynchronously, but we may call Delegate::DidRead synchronously? That seems like a bad idea. Always calling DidRead asynchronously doesn't seem like a great idea, either. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:275: void HttpConnection::CloseInNextRunLoop() { I think this and Close() are misnamed. Close() should be named CloseInNextRunLoop (Or better yet, CloseAsync), and this should be called close. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:23: Could you please document these classes? https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:24: class HttpConnection : public base::SupportsWeakPtr<HttpConnection> { WeakPtrs should generally not be exposed to external classes like this. Use a WeakPtrFactory instead. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:34: virtual void DidClose(HttpConnection* conn) = 0; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:34: virtual void DidClose(HttpConnection* conn) = 0; Write out connection (Google style guide discourages uncommon abbreviations) https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:45: size_t GetUnconsumedSize() const; IOBuffers use ints everything. I think mixing size_t's with ints is just a bad idea. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:71: size_t consumed_offset_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:8: #include <list> I don't believe this is used? (And wasn't before, either) https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:11: #include "base/basictypes.h" Should use macros.h instead (To make sure we get both OVERRIDE and DISALLOW_COPY_AND_ASSIGN) https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:27: public base::SupportsWeakPtr<HttpServer> { WeakPtrs shouldn't be exposed to external classes like this. Use WeakPtrFactory instead. https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:27: public base::SupportsWeakPtr<HttpServer> { This class should be declared with NEXT_EXPORT, shouldn't it? I'm not entirely sure why things are linking without that, actually. https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket.cc File net/socket/tcp_server_socket.cc (right): https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.cc:114: if (server_socket->Listen(net::IPEndPoint(ip_number, 0), 3) != OK) { You should be passing in the port here. https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.cc:114: if (server_socket->Listen(net::IPEndPoint(ip_number, 0), 3) != OK) { Having a hard-coded backlog here seems like a bad idea, and pretty non-obvious as well. https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket.h File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.h:14: #include "net/base/net_util.h" What is this needed for? https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.h:32: static scoped_ptr<TCPServerSocket> Create(const std::string& ip, int port); I think it's kinda of weird to have TCPServerSocket() and Listen() combined, but only when using this special alternative function, which takes in address in a different format. If it's worth adding a convenience function that takes the ip as a string, I think it should be added as an alternative Listen function, to ServerSocket. The implementation should also be in ServerSocket, since it just calls into Listen. You should also modify existing callers that could benefit from the function to use it (See extensions/browser/api/socket/tcp_socket.cc).
https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:217: if (pending_write_buf_->total_size() + data.size() > kPendingDataLimit) { Before we go further, could you elaborate on this limitation though? DevTools is sending 100Mb payloads using this method.
https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:217: if (pending_write_buf_->total_size() + data.size() > kPendingDataLimit) { On 2014/05/26 09:21:31, pfeldman wrote: > Before we go further, could you elaborate on this limitation though? DevTools is > sending 100Mb payloads using this method. It's buffering 100 MB payloads in RAM? That just seems a bad idea.
> It's buffering 100 MB payloads in RAM? That just seems a bad idea. Well, profiling tools tend to collect a lot of data in RAM. Then we need to flush it over the wire and reasonable socket interface would either buffer or tell us if it flushed partially. The suggested interface does not seem to provide us with any of the options. Or what am I missing?
On 2014/05/27 14:59:15, pfeldman wrote: > > It's buffering 100 MB payloads in RAM? That just seems a bad idea. > > Well, profiling tools tend to collect a lot of data in RAM. Then we need to > flush it over the wire and reasonable socket interface would either buffer or > tell us if it flushed partially. The suggested interface does not seem to > provide us with any of the options. Or what am I missing? Okay, can you send me the place where that happen? It sounds like HttpServer::Send() should be asynchronous which will make this CL really big.
On 2014/05/27 14:59:15, pfeldman wrote: > > It's buffering 100 MB payloads in RAM? That just seems a bad idea. > > Well, profiling tools tend to collect a lot of data in RAM. Then we need to > flush it over the wire and reasonable socket interface would either buffer or > tell us if it flushed partially. The suggested interface does not seem to > provide us with any of the options. Or what am I missing? Ahh...I had assumed the large size was for received response bodies from the cache. I still think 100+ MB of data in the browser process is a bad idea, from a "we really don't want to consume too much RAM and die" standpoint, but sounds a bit more reasonable if we're not streaming from a cache (be it disk or in-memory).
On 2014/05/27 16:03:42, byungchul wrote: > On 2014/05/27 14:59:15, pfeldman wrote: > > > It's buffering 100 MB payloads in RAM? That just seems a bad idea. > > > > Well, profiling tools tend to collect a lot of data in RAM. Then we need to > > flush it over the wire and reasonable socket interface would either buffer or > > tell us if it flushed partially. The suggested interface does not seem to > > provide us with any of the options. Or what am I missing? > > Okay, can you send me the place where that happen? It sounds like > HttpServer::Send() should be asynchronous which will make this CL really big. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de... Is this the place where it happens? Do you mean message could be big up to 100Mb?
On 2014/05/27 16:09:55, byungchul wrote: > On 2014/05/27 16:03:42, byungchul wrote: > > On 2014/05/27 14:59:15, pfeldman wrote: > > > > It's buffering 100 MB payloads in RAM? That just seems a bad idea. > > > > > > Well, profiling tools tend to collect a lot of data in RAM. Then we need to > > > flush it over the wire and reasonable socket interface would either buffer > or > > > tell us if it flushed partially. The suggested interface does not seem to > > > provide us with any of the options. Or what am I missing? > > > > Okay, can you send me the place where that happen? It sounds like > > HttpServer::Send() should be asynchronous which will make this CL really big. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de... > Is this the place where it happens? Do you mean message could be big up to > 100Mb? After a brief code browsing, it seems like all messages are transmitted via renderer process IPC with DevToolsClientMsg_DispatchOnInspectorFrontend, for example, https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/d... . Is it correct? If so, can each message have 100Mb? No matter how much data profiling tools generate, I only concern about the size of each message. So, the question is how big the size of each message of HttpServer::Send() could be. Is it 100Mb or more?
Yep, DevToolsClientMsg_DispatchOnInspectorFrontend can carry 100Mb.
On 2014/05/27 20:07:07, pfeldman wrote: > Yep, DevToolsClientMsg_DispatchOnInspectorFrontend can carry 100Mb. I see. Then, instead of changing all http server's apis asynchronous, how about introducing "int SendAsyncOverWebSocket(int connection_id, const std::string& data, const CompletionCallback& callback)"?
On 2014/05/27 23:33:05, byungchul wrote: > On 2014/05/27 20:07:07, pfeldman wrote: > > Yep, DevToolsClientMsg_DispatchOnInspectorFrontend can carry 100Mb. > > I see. Then, instead of changing all http server's apis asynchronous, how about > introducing "int SendAsyncOverWebSocket(int connection_id, const std::string& > data, const CompletionCallback& callback)"? Or, how about introducing "int SetSendBufferSize(int connection_id, int32 size)"?
On 2014/05/27 23:33:05, byungchul wrote: > On 2014/05/27 20:07:07, pfeldman wrote: > > Yep, DevToolsClientMsg_DispatchOnInspectorFrontend can carry 100Mb. > > I see. Then, instead of changing all http server's apis asynchronous, how about > introducing "int SendAsyncOverWebSocket(int connection_id, const std::string& > data, const CompletionCallback& callback)"? I don't think we want to go this route. We really want consistency with the rest of net/, which is all async. Also, strong +1 to Matt's comments re: IOBuffer
On 2014/05/28 00:08:30, byungchul wrote: > On 2014/05/27 23:33:05, byungchul wrote: > > On 2014/05/27 20:07:07, pfeldman wrote: > > > Yep, DevToolsClientMsg_DispatchOnInspectorFrontend can carry 100Mb. > > > > I see. Then, instead of changing all http server's apis asynchronous, how > about > > introducing "int SendAsyncOverWebSocket(int connection_id, const std::string& > > data, const CompletionCallback& callback)"? > > Or, how about introducing "int SetSendBufferSize(int connection_id, int32 > size)"? A more tenable proposal. Do we really want it per socket though? Or per server?
Replied on comments before changing code. https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... File content/browser/devtools/devtools_browser_target.cc (right): https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... content/browser/devtools/devtools_browser_target.cc:139: if (!http_server_) On 2014/05/22 06:01:05, pfeldman wrote: > if (!http_server_->get()) weak_ptr is testable. https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... File content/browser/devtools/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... content/browser/devtools/devtools_http_handler_impl.cc:121: base::WeakPtr<net::HttpServer> server_; On 2014/05/22 06:01:05, pfeldman wrote: > It was not refcounting it, so you should be fine with leaving it as raw. It is used in base::Bind() not to call when server is gone. Need to be weak_ptr here. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:125: pending_data_.push(data); On 2014/05/23 19:20:58, mmenke wrote: > Can't we just take IOBuffers instead, to avoid this copy? To construct IOBuffer from std::string in any point before here, it must be StringIOBuffer() which also uses std::string's ctor internally. So, same behavior unless HttpServer::Send() signature changes. And, no copy if std::string implements copy-on-write. And, t https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:157: DCHECK_GT(pending_data_.front().size(), consumed); On 2014/05/23 19:20:58, mmenke wrote: > Actually...Why don't we do something simpler, like just not supporting > pipelining, and only having one send call until the previous one completes? Any one needs to store date to write anyway. My implementation is here. Do you mean http server, or http connection? https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:267: void HttpConnection::Close() { On 2014/05/23 19:20:58, mmenke wrote: > This function can end up being called twice: Once on read error, once on write > error, which could result in calling into the delegate twice, which seems like a > potential problem. That's why I use a weak ptr. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:272: base::Bind(&HttpConnection::CloseInNextRunLoop, AsWeakPtr())); On 2014/05/23 19:20:58, mmenke wrote: > So we always call back Delegate::DidClose asynchronously, but we may call > Delegate::DidRead synchronously? That seems like a bad idea. Always calling > DidRead asynchronously doesn't seem like a great idea, either. They are different. DidClose() could be called for Close(), but DidRead() doesn't have a corresponding api. So, hard to say it is synchronous. https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:27: public base::SupportsWeakPtr<HttpServer> { On 2014/05/23 19:20:58, mmenke wrote: > This class should be declared with NEXT_EXPORT, shouldn't it? I'm not entirely > sure why things are linking without that, actually. NEXT_EXPORT is only for shared lib. Not sure which platforms build all shared libs separately (libbase.so, libnet.so, ...). Probably no platfroms do, and/so this is not verified well.
On 2014/05/28 00:17:08, Ryan Sleevi wrote: > On 2014/05/27 23:33:05, byungchul wrote: > > On 2014/05/27 20:07:07, pfeldman wrote: > > > Yep, DevToolsClientMsg_DispatchOnInspectorFrontend can carry 100Mb. > > > > I see. Then, instead of changing all http server's apis asynchronous, how > about > > introducing "int SendAsyncOverWebSocket(int connection_id, const std::string& > > data, const CompletionCallback& callback)"? > > I don't think we want to go this route. > > We really want consistency with the rest of net/, which is all async. > > Also, strong +1 to Matt's comments re: IOBuffer This CL is to keep HttpServer interface as much as possible. All async interface might be the next step. Otherwise, this CL could be too big to manage.
https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... File content/browser/devtools/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... content/browser/devtools/devtools_http_handler_impl.cc:121: base::WeakPtr<net::HttpServer> server_; On 2014/05/28 01:19:35, byungchul wrote: > On 2014/05/22 06:01:05, pfeldman wrote: > > It was not refcounting it, so you should be fine with leaving it as raw. > > It is used in base::Bind() not to call when server is gone. Need to be weak_ptr > here. Are you sure it's necessary? We may be able to get away with a base::Unretained(this), which is possible when the callback-inducing class has reasonable guarantees (which we've tried to do in net/, which is that we don't PostTask deferrals but instead invoke the callbacks during processing, which has the side-effect of guaranteeing that no callbacks are invoked once the owning object deletes it) https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:125: pending_data_.push(data); On 2014/05/28 01:19:35, byungchul wrote: > On 2014/05/23 19:20:58, mmenke wrote: > > Can't we just take IOBuffers instead, to avoid this copy? > > To construct IOBuffer from std::string in any point before here, it must be > StringIOBuffer() which also uses std::string's ctor internally. So, same > behavior unless HttpServer::Send() signature changes. > And, no copy if std::string implements copy-on-write. > And, t Copy-on-write is forbidden behaviour in C++11, and was never reliable behaviour before. I think the goal (of IOBuffer) is that IOBuffer is viral all the layers up. That is, changing multiple layers to take either a net::IOBuffer or (if a copy is absolutely necessary), base::StringPiece. Preferably though, IOBuffer through and through. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:157: DCHECK_GT(pending_data_.front().size(), consumed); On 2014/05/28 01:19:35, byungchul wrote: > On 2014/05/23 19:20:58, mmenke wrote: > > Actually...Why don't we do something simpler, like just not supporting > > pipelining, and only having one send call until the previous one completes? > > Any one needs to store date to write anyway. My implementation is here. Do you > mean http server, or http connection? Normally, in net/ code, we leave buffering up to the highest layer possible, so that it can adapt the most reasonable backoff metrics. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:267: void HttpConnection::Close() { On 2014/05/28 01:19:35, byungchul wrote: > On 2014/05/23 19:20:58, mmenke wrote: > > This function can end up being called twice: Once on read error, once on > write > > error, which could result in calling into the delegate twice, which seems like > a > > potential problem. > > That's why I use a weak ptr. I don't really think that's an ideal solution. Note that in net, we use the (spiritual equivalent, sometimes literal) of base::ResetAndReturn(cb_).Run(value) as the terminal call in the callstack, so that everything is unwound in the socket. Even if cb_ deletes the socket, it's safe, because it was stack allocated in the processing. eg: See https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc... or https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:24: class HttpConnection : public base::SupportsWeakPtr<HttpConnection> { On 2014/05/23 19:20:58, mmenke wrote: > WeakPtrs should generally not be exposed to external classes like this. Use a > WeakPtrFactory instead. +1 Note that WeakPtrFactory should be the *last* member of the class. https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:27: public base::SupportsWeakPtr<HttpServer> { On 2014/05/28 01:19:35, byungchul wrote: > On 2014/05/23 19:20:58, mmenke wrote: > > This class should be declared with NEXT_EXPORT, shouldn't it? I'm not > entirely > > sure why things are linking without that, actually. > > NEXT_EXPORT is only for shared lib. Not sure which platforms build all shared > libs separately (libbase.so, libnet.so, ...). Probably no platfroms do, and/so > this is not verified well. All of our platforms do component builds on the waterfall. It's only Chrome releases that do static builds. So yes, Matt's comment applies - since you won't be able to pass trybots if this class is used from a test (because net_unittests links against net.so)
https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:157: DCHECK_GT(pending_data_.front().size(), consumed); On 2014/05/28 01:36:26, Ryan Sleevi wrote: > On 2014/05/28 01:19:35, byungchul wrote: > > On 2014/05/23 19:20:58, mmenke wrote: > > > Actually...Why don't we do something simpler, like just not supporting > > > pipelining, and only having one send call until the previous one completes? > > > > Any one needs to store date to write anyway. My implementation is here. Do you > > mean http server, or http connection? > > Normally, in net/ code, we leave buffering up to the highest layer possible, so > that it can adapt the most reasonable backoff metrics. While keeping current HttpServer apis, I will try to do: 1) move read/write function to HttpServer and handle callbacks directly which will remove necessity of delegate and/so weak ptr. 2) HttpConnection is still useful as a container of information of each http connection including id, underlying socket, and pending buffer for read and write. Make sense?
PTAL. 1) Moved async read/write to HttpServer. 2) HttpConnection is just a container. 3) Sets send buffer of devtools websocket to 100Mb. 3) unix socket implementation is still missing. https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/net... File chrome/test/chromedriver/net/test_http_server.cc (right): https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/net... chrome/test/chromedriver/net/test_http_server.cc:133: EXPECT_NE(NULL, server_.get()); On 2014/05/23 19:20:58, mmenke wrote: > If this fails, we'll crash 2 lines down. > > Suggest: > > *success = false; > ASSERT_NE(...) Removed. https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/ser... File chrome/test/chromedriver/server/chromedriver_server.cc (right): https://codereview.chromium.org/296053012/diff/1/chrome/test/chromedriver/ser... chrome/test/chromedriver/server/chromedriver_server.cc:58: if (!server_) { On 2014/05/22 06:01:05, pfeldman_ooo wrote: > style nit: we don't put {} around single line blocks. Done. https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... File content/browser/devtools/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/296053012/diff/1/content/browser/devtools/dev... content/browser/devtools/devtools_http_handler_impl.cc:121: base::WeakPtr<net::HttpServer> server_; On 2014/05/28 01:36:26, Ryan Sleevi wrote: > On 2014/05/28 01:19:35, byungchul wrote: > > On 2014/05/22 06:01:05, pfeldman wrote: > > > It was not refcounting it, so you should be fine with leaving it as raw. > > > > It is used in base::Bind() not to call when server is gone. Need to be > weak_ptr > > here. > > Are you sure it's necessary? We may be able to get away with a > base::Unretained(this), which is possible when the callback-inducing class has > reasonable guarantees (which we've tried to do in net/, which is that we don't > PostTask deferrals but instead invoke the callbacks during processing, which has > the side-effect of guaranteeing that no callbacks are invoked once the owning > object deletes it) It's challenging to make HttpServer callback-driven, especially for reading. So, let's keep current model for a while. then, yes, weak_ptr is necessary. devtools is running on a separate thread, and not guaranteed its existence. I think that's the reason why http server was ref-counted object before. But, it doesn't have to be ref-counted because it is dereferenced only on the devtools thread. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.cc File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:125: pending_data_.push(data); On 2014/05/28 01:36:26, Ryan Sleevi wrote: > On 2014/05/28 01:19:35, byungchul wrote: > > On 2014/05/23 19:20:58, mmenke wrote: > > > Can't we just take IOBuffers instead, to avoid this copy? > > > > To construct IOBuffer from std::string in any point before here, it must be > > StringIOBuffer() which also uses std::string's ctor internally. So, same > > behavior unless HttpServer::Send() signature changes. > > And, no copy if std::string implements copy-on-write. > > And, t > > Copy-on-write is forbidden behaviour in C++11, and was never reliable behaviour > before. > > I think the goal (of IOBuffer) is that IOBuffer is viral all the layers up. That > is, changing multiple layers to take either a net::IOBuffer or (if a copy is > absolutely necessary), base::StringPiece. Preferably though, IOBuffer through > and through. Now, HttpServer uses this buffer directly. Building IOBuffer() just make an overhead. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:157: DCHECK_GT(pending_data_.front().size(), consumed); On 2014/05/28 05:01:33, byungchul wrote: > On 2014/05/28 01:36:26, Ryan Sleevi wrote: > > On 2014/05/28 01:19:35, byungchul wrote: > > > On 2014/05/23 19:20:58, mmenke wrote: > > > > Actually...Why don't we do something simpler, like just not supporting > > > > pipelining, and only having one send call until the previous one > completes? > > > > > > Any one needs to store date to write anyway. My implementation is here. Do > you > > > mean http server, or http connection? > > > > Normally, in net/ code, we leave buffering up to the highest layer possible, > so > > that it can adapt the most reasonable backoff metrics. > > While keeping current HttpServer apis, I will try to do: > 1) move read/write function to HttpServer and handle callbacks directly which > will remove necessity of delegate and/so weak ptr. > 2) HttpConnection is still useful as a container of information of each http > connection including id, underlying socket, and pending buffer for read and > write. > > Make sense? Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:175: void HttpConnection::DoReadLoop(int rv) { On 2014/05/23 19:20:58, mmenke wrote: > Taking in an "rv" that's always OK seems a little odd. Should either get rid of > the parameter, or restructure the loop so it takes the rv of the last read as > input (The latter is a little complicated, because on the first call, it will be > 0, which is the same value we get on EOF). Done and moved to HttpServer. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:191: AsWeakPtr())); On 2014/05/23 19:20:58, mmenke wrote: > We don't need weak pointers here - we own the socket, and it won't call us after > we delete it. Replaced with connection id in http server. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:217: if (pending_write_buf_->total_size() + data.size() > kPendingDataLimit) { On 2014/05/27 14:32:08, mmenke wrote: > On 2014/05/26 09:21:31, pfeldman wrote: > > Before we go further, could you elaborate on this limitation though? DevTools > is > > sending 100Mb payloads using this method. > > It's buffering 100 MB payloads in RAM? That just seems a bad idea. Increased websocket buffer to 100Mb in cast of devtools websocket. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:231: Send(std::string(bytes, len)); On 2014/05/23 19:20:58, mmenke wrote: > This copy really shouldn't be needed. This is used only by net/server/web_socket.cc. One for header(16 bytes), one for framing (2 bytes). Remove this function, and copy happens in websocket.cc (or caller) which can optimize if necessary while keeping http server api simple. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:238: void HttpConnection::DoWriteLoop(int rv) { On 2014/05/23 19:20:58, mmenke wrote: > Again, rv is always "OK", so doesn't seem like it should be needed. Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:243: AsWeakPtr())); On 2014/05/23 19:20:58, mmenke wrote: > WeakPtr not needed. Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:267: void HttpConnection::Close() { On 2014/05/28 01:36:26, Ryan Sleevi wrote: > On 2014/05/28 01:19:35, byungchul wrote: > > On 2014/05/23 19:20:58, mmenke wrote: > > > This function can end up being called twice: Once on read error, once on > > write > > > error, which could result in calling into the delegate twice, which seems > like > > a > > > potential problem. > > > > That's why I use a weak ptr. > > I don't really think that's an ideal solution. > > Note that in net, we use the (spiritual equivalent, sometimes literal) of > base::ResetAndReturn(cb_).Run(value) as the terminal call in the callstack, so > that everything is unwound in the socket. Even if cb_ deletes the socket, it's > safe, because it was stack allocated in the processing. > > eg: See > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc... > or > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... HttpServer handles it and/so never calls Close() twice because it removes it from id_connection_map. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.c... net/server/http_connection.cc:275: void HttpConnection::CloseInNextRunLoop() { On 2014/05/23 19:20:58, mmenke wrote: > I think this and Close() are misnamed. Close() should be named > CloseInNextRunLoop (Or better yet, CloseAsync), and this should be called close. Not necessary. Removed. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:23: On 2014/05/23 19:20:58, mmenke wrote: > Could you please document these classes? Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:24: class HttpConnection : public base::SupportsWeakPtr<HttpConnection> { On 2014/05/28 01:36:26, Ryan Sleevi wrote: > On 2014/05/23 19:20:58, mmenke wrote: > > WeakPtrs should generally not be exposed to external classes like this. Use a > > WeakPtrFactory instead. > > +1 > > Note that WeakPtrFactory should be the *last* member of the class. Removed here, and changed in HttpServer. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:34: virtual void DidClose(HttpConnection* conn) = 0; On 2014/05/23 19:20:58, mmenke wrote: > Write out connection (Google style guide discourages uncommon abbreviations) Removed. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:45: size_t GetUnconsumedSize() const; On 2014/05/23 19:20:58, mmenke wrote: > IOBuffers use ints everything. I think mixing size_t's with ints is just a bad > idea. Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_connection.h... net/server/http_connection.h:71: size_t consumed_offset_; On 2014/05/23 19:20:58, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:8: #include <list> On 2014/05/23 19:20:58, mmenke wrote: > I don't believe this is used? (And wasn't before, either) Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:11: #include "base/basictypes.h" On 2014/05/23 19:20:58, mmenke wrote: > Should use macros.h instead (To make sure we get both OVERRIDE and > DISALLOW_COPY_AND_ASSIGN) Done. https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:27: public base::SupportsWeakPtr<HttpServer> { On 2014/05/28 01:36:26, Ryan Sleevi wrote: > On 2014/05/28 01:19:35, byungchul wrote: > > On 2014/05/23 19:20:58, mmenke wrote: > > > This class should be declared with NEXT_EXPORT, shouldn't it? I'm not > > entirely > > > sure why things are linking without that, actually. > > > > NEXT_EXPORT is only for shared lib. Not sure which platforms build all shared > > libs separately (libbase.so, libnet.so, ...). Probably no platfroms do, and/so > > this is not verified well. > > All of our platforms do component builds on the waterfall. It's only Chrome > releases that do static builds. > > So yes, Matt's comment applies - since you won't be able to pass trybots if this > class is used from a test (because net_unittests links against net.so) Done. https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket.cc File net/socket/tcp_server_socket.cc (right): https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.cc:114: if (server_socket->Listen(net::IPEndPoint(ip_number, 0), 3) != OK) { On 2014/05/23 19:20:58, mmenke wrote: > Having a hard-coded backlog here seems like a bad idea, and pretty non-obvious > as well. Done. https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket.h File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.h:14: #include "net/base/net_util.h" On 2014/05/23 19:20:58, mmenke wrote: > What is this needed for? Done. https://codereview.chromium.org/296053012/diff/1/net/socket/tcp_server_socket... net/socket/tcp_server_socket.h:32: static scoped_ptr<TCPServerSocket> Create(const std::string& ip, int port); On 2014/05/23 19:20:58, mmenke wrote: > I think it's kinda of weird to have TCPServerSocket() and Listen() combined, but > only when using this special alternative function, which takes in address in a > different format. > > If it's worth adding a convenience function that takes the ip as a string, I > think it should be added as an alternative Listen function, to ServerSocket. > The implementation should also be in ServerSocket, since it just calls into > Listen. > > You should also modify existing callers that could benefit from the function to > use it (See extensions/browser/api/socket/tcp_socket.cc). Defined ServerSocketFactory which is necessary to store ip/port info and create/listen later for devtools. Added TODO for tcp_socket.cc.
Sorry for slowness, I'll do another pass tomorrow morning.
Quick comments (Sorry again for slowness). Hope to do a more thorough pass this afternoon. https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:30: class ReadIOBuffer : public IOBuffer { Why can't we just use GrowableIOBuffer? https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.c... net/server/http_server.cc:79: if (connection->write_buf()->Append(data) && !writing_in_progress) This seems like a weird division of labor to me. Seems much more natural to put all this logic related to reading/sending (And drive the WriteLoop) in the HttpConnection itself, rather than in the HttpServer class. Same with DidRead. That way, all logic involved in how to handle appending data is in one place, instead of strewn across both files, and having this class muck about the guts of the HttpConnection. Alternatively, HttpConnection could just be a struct with no internal logic, rather than a class. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:8: #include <map> need std::string. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:13: #include "net/server/http_connection.h" Can just forward declare this, I believe. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:57: const std::string& data, I still think we should really be taking IOBuffers here. That would mean that we'd need a second IOBuffer for just the headers, but that would allow fewer copies, for smart consumers. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:70: base::WeakPtr<HttpServer> GetWeakPtr(); It's generally a bad idea to expose weak pointers externally, unless it's an expected part of the class's externally visible behavior. If DevTools really needs one, it should create its own. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:108: DISALLOW_COPY_AND_ASSIGN(HttpServer); Should include macros.h https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:38: class NET_EXPORT ServerSocketFactory { What's the motivation behind adding this class? https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:46: // right behavior. nit: "a right behavior" -> "the right behavior" https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:49: void set_address(const IPEndPoint& address) { address_ = address; } It seems odd to me that this isn't an argument to CreateAndListen. https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:50: void set_backlog(int backlog) { backlog_ = backlog; } These should be documented.
https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:30: class ReadIOBuffer : public IOBuffer { On 2014/06/04 16:29:50, mmenke wrote: > Why can't we just use GrowableIOBuffer? GrowableIOBuffer can append more data at the end of data, but cannot move the beginning. I want to move beginning pointer without actual data movement. That's the "consumed data" consumed by the caller. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.c... net/server/http_server.cc:79: if (connection->write_buf()->Append(data) && !writing_in_progress) On 2014/06/04 16:29:50, mmenke wrote: > This seems like a weird division of labor to me. Seems much more natural to put > all this logic related to reading/sending (And drive the WriteLoop) in the > HttpConnection itself, rather than in the HttpServer class. > > Same with DidRead. > > That way, all logic involved in how to handle appending data is in one place, > instead of strewn across both files, and having this class muck about the guts > of the HttpConnection. > > Alternatively, HttpConnection could just be a struct with no internal logic, > rather than a class. Yes, current HttpConnection is just a struct though it is still named "class". No internal logic for HttpConnection itself except ctor. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:57: const std::string& data, On 2014/06/04 16:29:50, mmenke wrote: > I still think we should really be taking IOBuffers here. That would mean that > we'd need a second IOBuffer for just the headers, but that would allow fewer > copies, for smart consumers. By nature of IOBuffer which doesn't have size(), it is shared by caller and callee. caller just hands the ownership of [IOBuffer->data(), IOBuffer->data() + buf_size passed in function] portion over to the callee. Other portion is still owned by the caller. From this nature, it should be used with callback for callee to hand the ownership back to the caller. So, we have options: 1) Use a specific IOBuffer, for example, StringIOBuffer which copies data once already. I don't see any benefits in terms of performance. 2) Take the ownership for all portion of IOBuffer and never returns back to the caller, where caller must create IOBuffer each time and copy data (again). Otherwise, it is hard for callee to maintain the lifetime of internal data of IOBuffer. I think this can lead confusions and errors. 3) Change http server model to accept IOBuffer, size and callback as other interfaces, where caller must implement buffering like one of this CL. If IOBuffer is only for performace, I don't think we can gain much. If it is to unify the api, can we do it in next CL? https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:70: base::WeakPtr<HttpServer> GetWeakPtr(); On 2014/06/04 16:29:50, mmenke wrote: > It's generally a bad idea to expose weak pointers externally, unless it's an > expected part of the class's externally visible behavior. > > If DevTools really needs one, it should create its own. DevTools posts tasks on http server. Do you mean devtools should post tasks on itself and call http server's functions? Though it sounds inefficient, doable. By the way, how can we determine whether or not "it's an expected part of the class's externally visible behavior"? https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:38: class NET_EXPORT ServerSocketFactory { On 2014/06/04 16:29:50, mmenke wrote: > What's the motivation behind adding this class? Devtools builds a socket factory class in UI thread, and instantiate a server/listen socket in its own thread. Devtools gets tcp server socket or unix domain server socket (for android). https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:49: void set_address(const IPEndPoint& address) { address_ = address; } On 2014/06/04 16:29:50, mmenke wrote: > It seems odd to me that this isn't an argument to CreateAndListen. Again, setting address and calling CreateAndListen() could happen in different thread. For devtools, they are UI thread and devtools own thread respectively.
https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:30: class ReadIOBuffer : public IOBuffer { On 2014/06/04 17:41:28, byungchul wrote: > On 2014/06/04 16:29:50, mmenke wrote: > > Why can't we just use GrowableIOBuffer? > > GrowableIOBuffer can append more data at the end of data, but cannot move the > beginning. I want to move beginning pointer without actual data movement. That's > the "consumed data" consumed by the caller. Growable IOBuffer has an "offset" which is the "beginning", and can be set directly...Everything before that has been consumed. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:57: const std::string& data, On 2014/06/04 17:41:28, byungchul wrote: > On 2014/06/04 16:29:50, mmenke wrote: > > I still think we should really be taking IOBuffers here. That would mean that > > we'd need a second IOBuffer for just the headers, but that would allow fewer > > copies, for smart consumers. > > By nature of IOBuffer which doesn't have size(), it is shared by caller and > callee. caller just hands the ownership of [IOBuffer->data(), IOBuffer->data() + > buf_size passed in function] portion over to the callee. Other portion is still > owned by the caller. From this nature, it should be used with callback for > callee to hand the ownership back to the caller. > So, we have options: > 1) Use a specific IOBuffer, for example, StringIOBuffer which copies data once > already. I don't see any benefits in terms of performance. > 2) Take the ownership for all portion of IOBuffer and never returns back to the > caller, where caller must create IOBuffer each time and copy data (again). > Otherwise, it is hard for callee to maintain the lifetime of internal data of > IOBuffer. I think this can lead confusions and errors. > 3) Change http server model to accept IOBuffer, size and callback as other > interfaces, where caller must implement buffering like one of this CL. > > If IOBuffer is only for performace, I don't think we can gain much. If it is to > unify the api, can we do it in next CL? Currently, things are like this: Embedder creates a string. We copy the string into HttpServerResponse info. We then serialize the string and headers in a single buffer. We then copy them into the IOBuffer. This is 3 extra copies, none of which are necessary. And when we're talking about 100MB (!) of data, this just seems like a really bad idea. If the embedder passed us an IOBuffer (And a length), then there would be no extra copies, if the embedder created the data in the IOBuffer directly, or one extra copy, if it created a string, and copied it to the IOBuffer. Alternatively, we could keep the current API and just do a single copy, straight into an IOBuffer, and put the headers in another buffer. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:70: base::WeakPtr<HttpServer> GetWeakPtr(); On 2014/06/04 17:41:28, byungchul wrote: > On 2014/06/04 16:29:50, mmenke wrote: > > It's generally a bad idea to expose weak pointers externally, unless it's an > > expected part of the class's externally visible behavior. > > > > If DevTools really needs one, it should create its own. > > DevTools posts tasks on http server. Do you mean devtools should post tasks on > itself and call http server's functions? Though it sounds inefficient, doable. > By the way, how can we determine whether or not "it's an > expected part of the class's externally visible behavior"? If the purpose of the class is related to message passing in a safe manner, it might be a logical part of the API. That's a pretty rare case, however. There doesn't seem to be any reason net should have knowledge that the embedder (DevTools) wants to use it with a WeakPtr. In general, exposing WeakPtrs to external classes is an anti-pattern that should be avoided.
https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:71: class PendingWriteIOBuffer : public IOBuffer { This class really should be unit tested independently.
https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:30: class ReadIOBuffer : public IOBuffer { On 2014/06/04 19:27:50, mmenke wrote: > On 2014/06/04 17:41:28, byungchul wrote: > > On 2014/06/04 16:29:50, mmenke wrote: > > > Why can't we just use GrowableIOBuffer? > > > > GrowableIOBuffer can append more data at the end of data, but cannot move the > > beginning. I want to move beginning pointer without actual data movement. > That's > > the "consumed data" consumed by the caller. > > Growable IOBuffer has an "offset" which is the "beginning", and can be set > directly...Everything before that has been consumed. Sure, I know. It moves the offset/pointer up to where new data will be written. I need another offset/pointer indicating where data consumed by caller because the caller may not consume everything read before. That's why I need a special IOBuffer which has 3 parts, but not 2 parts as GrowableIOBuffer. https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:71: class PendingWriteIOBuffer : public IOBuffer { On 2014/06/04 19:28:52, mmenke wrote: > This class really should be unit tested independently. Sure, I added TODO in commit message. Will do TODOs in this CL once we reach agreement on this CL generally and update the commit message accordingly. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:57: const std::string& data, On 2014/06/04 19:27:50, mmenke wrote: > On 2014/06/04 17:41:28, byungchul wrote: > > On 2014/06/04 16:29:50, mmenke wrote: > > > I still think we should really be taking IOBuffers here. That would mean > that > > > we'd need a second IOBuffer for just the headers, but that would allow fewer > > > copies, for smart consumers. > > > > By nature of IOBuffer which doesn't have size(), it is shared by caller and > > callee. caller just hands the ownership of [IOBuffer->data(), IOBuffer->data() > + > > buf_size passed in function] portion over to the callee. Other portion is > still > > owned by the caller. From this nature, it should be used with callback for > > callee to hand the ownership back to the caller. > > So, we have options: > > 1) Use a specific IOBuffer, for example, StringIOBuffer which copies data once > > already. I don't see any benefits in terms of performance. > > 2) Take the ownership for all portion of IOBuffer and never returns back to > the > > caller, where caller must create IOBuffer each time and copy data (again). > > Otherwise, it is hard for callee to maintain the lifetime of internal data of > > IOBuffer. I think this can lead confusions and errors. > > 3) Change http server model to accept IOBuffer, size and callback as other > > interfaces, where caller must implement buffering like one of this CL. > > > > If IOBuffer is only for performace, I don't think we can gain much. If it is > to > > unify the api, can we do it in next CL? > > Currently, things are like this: Embedder creates a string. We copy the string > into HttpServerResponse info. We then serialize the string and headers in a > single buffer. We then copy them into the IOBuffer. This is 3 extra copies, > none of which are necessary. And when we're talking about 100MB (!) of data, > this just seems like a really bad idea. > > If the embedder passed us an IOBuffer (And a length), then there would be no > extra copies, if the embedder created the data in the IOBuffer directly, or one > extra copy, if it created a string, and copied it to the IOBuffer. > > Alternatively, we could keep the current API and just do a single copy, straight > into an IOBuffer, and put the headers in another buffer. The main purpose of this CL is replacing SteamListenSocket with ServerSocket. Changing interface can be done later because this implementation doesn't add copy more than before. I would take your alternative way if you don't mind.
Two more quick responses... Still intend need to continue dig through everything and get more feedback to you, so no need to rush to get another CL up for feedback. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:57: const std::string& data, On 2014/06/04 20:51:26, byungchul wrote: > On 2014/06/04 19:27:50, mmenke wrote: > > On 2014/06/04 17:41:28, byungchul wrote: > > > On 2014/06/04 16:29:50, mmenke wrote: > > > > I still think we should really be taking IOBuffers here. That would mean > > that > > > > we'd need a second IOBuffer for just the headers, but that would allow > fewer > > > > copies, for smart consumers. > > > > > > By nature of IOBuffer which doesn't have size(), it is shared by caller and > > > callee. caller just hands the ownership of [IOBuffer->data(), > IOBuffer->data() > > + > > > buf_size passed in function] portion over to the callee. Other portion is > > still > > > owned by the caller. From this nature, it should be used with callback for > > > callee to hand the ownership back to the caller. > > > So, we have options: > > > 1) Use a specific IOBuffer, for example, StringIOBuffer which copies data > once > > > already. I don't see any benefits in terms of performance. > > > 2) Take the ownership for all portion of IOBuffer and never returns back to > > the > > > caller, where caller must create IOBuffer each time and copy data (again). > > > Otherwise, it is hard for callee to maintain the lifetime of internal data > of > > > IOBuffer. I think this can lead confusions and errors. > > > 3) Change http server model to accept IOBuffer, size and callback as other > > > interfaces, where caller must implement buffering like one of this CL. > > > > > > If IOBuffer is only for performace, I don't think we can gain much. If it is > > to > > > unify the api, can we do it in next CL? > > > > Currently, things are like this: Embedder creates a string. We copy the > string > > into HttpServerResponse info. We then serialize the string and headers in a > > single buffer. We then copy them into the IOBuffer. This is 3 extra copies, > > none of which are necessary. And when we're talking about 100MB (!) of data, > > this just seems like a really bad idea. > > > > If the embedder passed us an IOBuffer (And a length), then there would be no > > extra copies, if the embedder created the data in the IOBuffer directly, or > one > > extra copy, if it created a string, and copied it to the IOBuffer. > > > > Alternatively, we could keep the current API and just do a single copy, > straight > > into an IOBuffer, and put the headers in another buffer. > > The main purpose of this CL is replacing SteamListenSocket with ServerSocket. > Changing interface can be done later because this implementation doesn't add > copy more than before. > I would take your alternative way if you don't mind. Longer term, I'd love to see it changed, but I'm happy with the alternative, for now. https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:38: class NET_EXPORT ServerSocketFactory { On 2014/06/04 17:41:28, byungchul wrote: > On 2014/06/04 16:29:50, mmenke wrote: > > What's the motivation behind adding this class? > > Devtools builds a socket factory class in UI thread, and instantiate a > server/listen socket in its own thread. > Devtools gets tcp server socket or unix domain server socket (for android). Hrm...Wonder if it makes more sense to make this DevTools class, then. I'm not strongly opposed to it here, just wondering if it really belongs in net/.
On 2014/06/04 20:59:59, mmenke wrote: > Two more quick responses... Still intend need to continue dig through > everything and get more feedback to you, so no need to rush to get another CL up > for feedback. > > https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h > File net/server/http_server.h (right): > > https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... > net/server/http_server.h:57: const std::string& data, > On 2014/06/04 20:51:26, byungchul wrote: > > On 2014/06/04 19:27:50, mmenke wrote: > > > On 2014/06/04 17:41:28, byungchul wrote: > > > > On 2014/06/04 16:29:50, mmenke wrote: > > > > > I still think we should really be taking IOBuffers here. That would > mean > > > that > > > > > we'd need a second IOBuffer for just the headers, but that would allow > > fewer > > > > > copies, for smart consumers. > > > > > > > > By nature of IOBuffer which doesn't have size(), it is shared by caller > and > > > > callee. caller just hands the ownership of [IOBuffer->data(), > > IOBuffer->data() > > > + > > > > buf_size passed in function] portion over to the callee. Other portion is > > > still > > > > owned by the caller. From this nature, it should be used with callback for > > > > callee to hand the ownership back to the caller. > > > > So, we have options: > > > > 1) Use a specific IOBuffer, for example, StringIOBuffer which copies data > > once > > > > already. I don't see any benefits in terms of performance. > > > > 2) Take the ownership for all portion of IOBuffer and never returns back > to > > > the > > > > caller, where caller must create IOBuffer each time and copy data (again). > > > > Otherwise, it is hard for callee to maintain the lifetime of internal data > > of > > > > IOBuffer. I think this can lead confusions and errors. > > > > 3) Change http server model to accept IOBuffer, size and callback as other > > > > interfaces, where caller must implement buffering like one of this CL. > > > > > > > > If IOBuffer is only for performace, I don't think we can gain much. If it > is > > > to > > > > unify the api, can we do it in next CL? > > > > > > Currently, things are like this: Embedder creates a string. We copy the > > string > > > into HttpServerResponse info. We then serialize the string and headers in a > > > single buffer. We then copy them into the IOBuffer. This is 3 extra > copies, > > > none of which are necessary. And when we're talking about 100MB (!) of > data, > > > this just seems like a really bad idea. > > > > > > If the embedder passed us an IOBuffer (And a length), then there would be no > > > extra copies, if the embedder created the data in the IOBuffer directly, or > > one > > > extra copy, if it created a string, and copied it to the IOBuffer. > > > > > > Alternatively, we could keep the current API and just do a single copy, > > straight > > > into an IOBuffer, and put the headers in another buffer. > > > > The main purpose of this CL is replacing SteamListenSocket with ServerSocket. > > Changing interface can be done later because this implementation doesn't add > > copy more than before. > > I would take your alternative way if you don't mind. > > Longer term, I'd love to see it changed, but I'm happy with the alternative, > for now. > > https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket.h > File net/socket/server_socket.h (right): > > https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... > net/socket/server_socket.h:38: class NET_EXPORT ServerSocketFactory { > On 2014/06/04 17:41:28, byungchul wrote: > > On 2014/06/04 16:29:50, mmenke wrote: > > > What's the motivation behind adding this class? > > > > Devtools builds a socket factory class in UI thread, and instantiate a > > server/listen socket in its own thread. > > Devtools gets tcp server socket or unix domain server socket (for android). > > Hrm...Wonder if it makes more sense to make this DevTools class, then. I'm not > strongly opposed to it here, just wondering if it really belongs in net/. In an ideal world, this would have either stayed on DevTools, or been refactored when moving to net/. There's no question that DevTools' behaviour is as non-ideal as other teams' usage of that. The question is, how do we move forward, and how has a strong sense of ownership to 1) Change this class 2) Fix the dependents My fear is that the push is just to do as little as 1 as possible, but that doesn't leave us in a better state.
Could you elaborate on what is not ideal in the DevTools behavior? We are happy to follow up and fix it.
On 2014/06/05 04:25:50, pfeldman_ooo wrote: > Could you elaborate on what is not ideal in the DevTools behavior? We are happy > to follow up and fix it. Ideally, we would have it using the IOThread and not blocking APIs. I think the end goal with net/server will have us moving to that model, but it's going to be a non-trivial effort.
There is nothing forcing us to use blocking apis. All we do is receive a string message over IPC and send it as a websocket frame. And vice versa, we get the websocket frame over http and send it over ipc. When you say IOThread, do you mean _the_ browser IO thread or message loop with type IO? We were specifically advised against the former and are using the latter.
now that i am trying to recollect why we could not use io thread, i am starting to think that it was due to listen_socket being the only server socket in the source base and it was unreliable and potentially blocking. anyways, we would be quite happy to migrate to io, just tell us where we could help.
I'm really thinking we need tests for this. https://codereview.chromium.org/296053012/diff/60001/net/server/http_connecti... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/60001/net/server/http_connecti... net/server/http_connection.cc:46: data_ = base_->StartOfBuffer(); This looks wrong to me, when there's unconsumed data. Should either copy unconsumed data to the start of the bugger (And update offset_ accordingly) first, or set data_ to be base_->StartOfBuffer() + [old value of data_ - base_->StartOfBuffer]. This code is sufficiently complicated that I don't think we should land it prior to adding unit tests.
Unittests for buffer management are ready. UnixDomainServerSocket is still in progress, but want to get some review first. https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_connecti... net/server/http_connection.h:30: class ReadIOBuffer : public IOBuffer { On 2014/06/04 20:51:26, byungchul wrote: > On 2014/06/04 19:27:50, mmenke wrote: > > On 2014/06/04 17:41:28, byungchul wrote: > > > On 2014/06/04 16:29:50, mmenke wrote: > > > > Why can't we just use GrowableIOBuffer? > > > > > > GrowableIOBuffer can append more data at the end of data, but cannot move > the > > > beginning. I want to move beginning pointer without actual data movement. > > That's > > > the "consumed data" consumed by the caller. > > > > Growable IOBuffer has an "offset" which is the "beginning", and can be set > > directly...Everything before that has been consumed. > > Sure, I know. It moves the offset/pointer up to where new data will be written. > I need another offset/pointer indicating where data consumed by caller because > the caller may not consume everything read before. That's why I need a special > IOBuffer which has 3 parts, but not 2 parts as GrowableIOBuffer. Revised comment. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:8: #include <map> On 2014/06/04 16:29:50, mmenke wrote: > need std::string. Done. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:13: #include "net/server/http_connection.h" On 2014/06/04 16:29:50, mmenke wrote: > Can just forward declare this, I believe. Done. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:57: const std::string& data, On 2014/06/04 21:00:00, mmenke wrote: > On 2014/06/04 20:51:26, byungchul wrote: > > On 2014/06/04 19:27:50, mmenke wrote: > > > On 2014/06/04 17:41:28, byungchul wrote: > > > > On 2014/06/04 16:29:50, mmenke wrote: > > > > > I still think we should really be taking IOBuffers here. That would > mean > > > that > > > > > we'd need a second IOBuffer for just the headers, but that would allow > > fewer > > > > > copies, for smart consumers. > > > > > > > > By nature of IOBuffer which doesn't have size(), it is shared by caller > and > > > > callee. caller just hands the ownership of [IOBuffer->data(), > > IOBuffer->data() > > > + > > > > buf_size passed in function] portion over to the callee. Other portion is > > > still > > > > owned by the caller. From this nature, it should be used with callback for > > > > callee to hand the ownership back to the caller. > > > > So, we have options: > > > > 1) Use a specific IOBuffer, for example, StringIOBuffer which copies data > > once > > > > already. I don't see any benefits in terms of performance. > > > > 2) Take the ownership for all portion of IOBuffer and never returns back > to > > > the > > > > caller, where caller must create IOBuffer each time and copy data (again). > > > > Otherwise, it is hard for callee to maintain the lifetime of internal data > > of > > > > IOBuffer. I think this can lead confusions and errors. > > > > 3) Change http server model to accept IOBuffer, size and callback as other > > > > interfaces, where caller must implement buffering like one of this CL. > > > > > > > > If IOBuffer is only for performace, I don't think we can gain much. If it > is > > > to > > > > unify the api, can we do it in next CL? > > > > > > Currently, things are like this: Embedder creates a string. We copy the > > string > > > into HttpServerResponse info. We then serialize the string and headers in a > > > single buffer. We then copy them into the IOBuffer. This is 3 extra > copies, > > > none of which are necessary. And when we're talking about 100MB (!) of > data, > > > this just seems like a really bad idea. > > > > > > If the embedder passed us an IOBuffer (And a length), then there would be no > > > extra copies, if the embedder created the data in the IOBuffer directly, or > > one > > > extra copy, if it created a string, and copied it to the IOBuffer. > > > > > > Alternatively, we could keep the current API and just do a single copy, > > straight > > > into an IOBuffer, and put the headers in another buffer. > > > > The main purpose of this CL is replacing SteamListenSocket with ServerSocket. > > Changing interface can be done later because this implementation doesn't add > > copy more than before. > > I would take your alternative way if you don't mind. > > Longer term, I'd love to see it changed, but I'm happy with the alternative, > for now. Done. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:70: base::WeakPtr<HttpServer> GetWeakPtr(); On 2014/06/04 19:27:50, mmenke wrote: > On 2014/06/04 17:41:28, byungchul wrote: > > On 2014/06/04 16:29:50, mmenke wrote: > > > It's generally a bad idea to expose weak pointers externally, unless it's an > > > expected part of the class's externally visible behavior. > > > > > > If DevTools really needs one, it should create its own. > > > > DevTools posts tasks on http server. Do you mean devtools should post tasks on > > itself and call http server's functions? Though it sounds inefficient, doable. > > By the way, how can we determine whether or not "it's an > > expected part of the class's externally visible behavior"? > > If the purpose of the class is related to message passing in a safe manner, it > might be a logical part of the API. That's a pretty rare case, however. > > There doesn't seem to be any reason net should have knowledge that the embedder > (DevTools) wants to use it with a WeakPtr. In general, exposing WeakPtrs to > external classes is an anti-pattern that should be avoided. Removed. Now devtools destroys objects which have the reference of http server before it destroys http server. https://codereview.chromium.org/296053012/diff/50001/net/server/http_server.h... net/server/http_server.h:108: DISALLOW_COPY_AND_ASSIGN(HttpServer); On 2014/06/04 16:29:50, mmenke wrote: > Should include macros.h Done. https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:38: class NET_EXPORT ServerSocketFactory { On 2014/06/04 21:00:00, mmenke wrote: > On 2014/06/04 17:41:28, byungchul wrote: > > On 2014/06/04 16:29:50, mmenke wrote: > > > What's the motivation behind adding this class? > > > > Devtools builds a socket factory class in UI thread, and instantiate a > > server/listen socket in its own thread. > > Devtools gets tcp server socket or unix domain server socket (for android). > > Hrm...Wonder if it makes more sense to make this DevTools class, then. I'm not > strongly opposed to it here, just wondering if it really belongs in net/. Done. https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:46: // right behavior. On 2014/06/04 16:29:50, mmenke wrote: > nit: "a right behavior" -> "the right behavior" Done. https://codereview.chromium.org/296053012/diff/50001/net/socket/server_socket... net/socket/server_socket.h:50: void set_backlog(int backlog) { backlog_ = backlog; } On 2014/06/04 16:29:50, mmenke wrote: > These should be documented. Removed and they are passed to constructor. https://codereview.chromium.org/296053012/diff/60001/net/server/http_connecti... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/60001/net/server/http_connecti... net/server/http_connection.cc:46: data_ = base_->StartOfBuffer(); On 2014/06/06 15:41:03, mmenke wrote: > This looks wrong to me, when there's unconsumed data. Should either copy > unconsumed data to the start of the bugger (And update offset_ accordingly) > first, or set data_ to be base_->StartOfBuffer() + [old value of data_ - > base_->StartOfBuffer]. > > This code is sufficiently complicated that I don't think we should land it prior > to adding unit tests. Fixed and added unittests.
PTAL. Added unittests for buffer management in HttpConnection, new unix domain server/client sockets.
https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/1/net/server/http_server.h#new... net/server/http_server.h:27: public base::SupportsWeakPtr<HttpServer> { On 2014/05/30 00:19:02, byungchul wrote: > On 2014/05/28 01:36:26, Ryan Sleevi wrote: > > On 2014/05/28 01:19:35, byungchul wrote: > > > On 2014/05/23 19:20:58, mmenke wrote: > > > > This class should be declared with NEXT_EXPORT, shouldn't it? I'm not > > > entirely > > > > sure why things are linking without that, actually. > > > > > > NEXT_EXPORT is only for shared lib. Not sure which platforms build all > shared > > > libs separately (libbase.so, libnet.so, ...). Probably no platfroms do, > and/so > > > this is not verified well. > > > > All of our platforms do component builds on the waterfall. It's only Chrome > > releases that do static builds. > > > > So yes, Matt's comment applies - since you won't be able to pass trybots if > this > > class is used from a test (because net_unittests links against net.so) > > Done. Turned out that http_server is built as a static lib always. See https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=1042 . So, remove NET_EXPORT in lastest patch set.
Could we split this into two CLS for simpler review? 2600 lines of code is just painful. The new unix domain sockets and socket_server changes could be landed separately, though admittedly, those aren't where all of the complexity is. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:23: IOBuffer* HttpConnection::ReadIOBuffer::GetUnusedIOBuffer() const { I think this is a very confusing name and concept. My suggestion: Make ReadIOBuffer inherit from GrowableIOBuffer, and get rid of this function. You'd have to rename the current data_ to something else, but I think the usage pattern would be clearer. I'd suggest just calling it unconsumed_data_. Open to other ideas. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:27: int HttpConnection::ReadIOBuffer::GetUnusedCapacity() const { Suggest keeping this as GetRemainingCapacity, for everyone's sanity. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:44: LOG(ERROR) << "Too large read data is pending: capacity=" << GetCapacity() DLOG, to avoid bloating up binaries. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:50: int new_capacity = GetCapacity() * kCapacityIncreaseFactor; Seems a little weird to me that we never make an effort to move unhandled requests back to the start of the buffer. I don't see this causing us problems in actual usage, but seems a somewhat weird behavior that we just resize the buffer instead of moving all unread data to the start of the buffer, when we can. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:147: DCHECK_EQ(total_size_, 0); nit: Expected goes before actual. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:5: #include "net/server/http_connection.h" nit: Line break after main header. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:88: const int read_length = 128; nit: kReadLength (Same goes for the others) https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:117: // Reads another data. nit: "Reads more data." https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:146: } Need a test where the buffer length is increased when some data has already been read. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:163: const std::string data_2("another data to write"); nit: "more data to write" https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socke... File net/socket/server_socket.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socke... net/socket/server_socket.cc:18: int ServerSocket::ListenWithAddressAndPort(const std::string& ip_address_string, nit: Use same variable names here as in the header. https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socke... net/socket/server_socket.h:29: // address. The last sentence is confusing - unclear what it's trying to say. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:77: int UnixDomainClientSocket::Connect(const CompletionCallback& callback) { DCHECK(waiting_connect_)? https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:79: return ERR_SOCKET_IS_CONNECTED; Should we DCHECK on this instead? (I know we don't do this elsewhere, but the Chrome pholosophy is to DCHECK and not handle coding bugs, unless they're security related) https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:85: DCHECK(write_callback_.is_null()); Move these up before the IsConnected check? https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:279: void UnixDomainClientSocket::DidCompleteWrite() { This function seems to be misnamed (As is DidCompleteRead). Same is true for tcp_socket_libevent, which this class closely resembles, but since it doesn't seem to go out of its way to be identical to tcp_socket_libevent... It's a bit unfortunate that we can't easily just reuse all the code for TCPClientSocketLibevent. It's not a huge amount of redundant code, but it does mean that any updates to one class won't affect the other. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:310: NOTREACHED(); Shouldn't be handling this case, should just have DCHECK (TCPSocketLibevent does the same thing, but believe the consensus now is that we should not handle cases that should never happen). Same goes for OnFileCanReadWithoutBlocking. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.h:70: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE; These aren't intended to be used by consumers, so shouldn't be public.
https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:23: IOBuffer* HttpConnection::ReadIOBuffer::GetUnusedIOBuffer() const { On 2014/06/11 16:05:49, mmenke wrote: > I think this is a very confusing name and concept. My suggestion: Make > ReadIOBuffer inherit from GrowableIOBuffer, and get rid of this function. > > You'd have to rename the current data_ to something else, but I think the usage > pattern would be clearer. I'd suggest just calling it unconsumed_data_. Open > to other ideas. Ignore this suggestion - I think we should think about how to make this simpler, but I don't think this is the way to go.
I also feel sorry that this CL is getting bigger. Will try to split. Please see the some comments before another patch set. And, thank you very much for reviewing. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:23: IOBuffer* HttpConnection::ReadIOBuffer::GetUnusedIOBuffer() const { On 2014/06/11 16:09:04, mmenke wrote: > On 2014/06/11 16:05:49, mmenke wrote: > > I think this is a very confusing name and concept. My suggestion: Make > > ReadIOBuffer inherit from GrowableIOBuffer, and get rid of this function. > > > > You'd have to rename the current data_ to something else, but I think the > usage > > pattern would be clearer. I'd suggest just calling it unconsumed_data_. Open > > to other ideas. > > Ignore this suggestion - I think we should think about how to make this simpler, > but I don't think this is the way to go. Okay, I am re-thinking about how to manage the read buffer. But, please understand that this depends on how http server and websocket handle packets. They assume read data buffer is continuous. Unless we change this behavior, it's hard to split read buffer in multiple segments to reduce data copy. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:50: int new_capacity = GetCapacity() * kCapacityIncreaseFactor; On 2014/06/11 16:05:49, mmenke wrote: > Seems a little weird to me that we never make an effort to move unhandled > requests back to the start of the buffer. I don't see this causing us problems > in actual usage, but seems a somewhat weird behavior that we just resize the > buffer instead of moving all unread data to the start of the buffer, when we > can. Yes, your suggestion would be helpful though this will be called again soon in practical. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:279: void UnixDomainClientSocket::DidCompleteWrite() { On 2014/06/11 16:05:50, mmenke wrote: > This function seems to be misnamed (As is DidCompleteRead). Same is true for > tcp_socket_libevent, which this class closely resembles, but since it doesn't > seem to go out of its way to be identical to tcp_socket_libevent... > > It's a bit unfortunate that we can't easily just reuse all the code for > TCPClientSocketLibevent. It's not a huge amount of redundant code, but it does > mean that any updates to one class won't affect the other. Per your request, will try to split this change into 2 CLs. One for unix stream socket is the prerequisite for the other of http server refactoring. Will define socket_libevent to be inherited by here and tcp_libevent.cc. Makes sense?
https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:23: IOBuffer* HttpConnection::ReadIOBuffer::GetUnusedIOBuffer() const { On 2014/06/11 17:30:28, byungchul wrote: > On 2014/06/11 16:09:04, mmenke wrote: > > On 2014/06/11 16:05:49, mmenke wrote: > > > I think this is a very confusing name and concept. My suggestion: Make > > > ReadIOBuffer inherit from GrowableIOBuffer, and get rid of this function. > > > > > > You'd have to rename the current data_ to something else, but I think the > > usage > > > pattern would be clearer. I'd suggest just calling it unconsumed_data_. > Open > > > to other ideas. > > > > Ignore this suggestion - I think we should think about how to make this > simpler, > > but I don't think this is the way to go. > > Okay, I am re-thinking about how to manage the read buffer. But, please > understand that this depends on how http server and websocket handle packets. > They assume read data buffer is continuous. Unless we change this behavior, it's > hard to split read buffer in multiple segments to reduce data copy. I'm not concerned with reducing data copy for the read buffer, actually, since we're just reading HTTP requests. I just think the class can be a bit hard to follow. Thinking more along the lines of improving naming / readability of ReadIOBuffer, than changing how it basically works. My move suggestion below was more aimed at limiting needed buffer size, in the worst case, rather than improving performance. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:279: void UnixDomainClientSocket::DidCompleteWrite() { On 2014/06/11 17:30:28, byungchul wrote: > On 2014/06/11 16:05:50, mmenke wrote: > > This function seems to be misnamed (As is DidCompleteRead). Same is true for > > tcp_socket_libevent, which this class closely resembles, but since it doesn't > > seem to go out of its way to be identical to tcp_socket_libevent... > > > > It's a bit unfortunate that we can't easily just reuse all the code for > > TCPClientSocketLibevent. It's not a huge amount of redundant code, but it > does > > mean that any updates to one class won't affect the other. > > Per your request, will try to split this change into 2 CLs. One for unix stream > socket is the prerequisite for the other of http server refactoring. Will define > socket_libevent to be inherited by here and tcp_libevent.cc. Makes sense? Yea, makes sense. Hrm... Since TCPSocketLibevent doesn't inherit from StreamSocket, I'm not sure the best way to do this cleanly.
CL for unix socket refactoring is uploaded in https://codereview.chromium.org/348803003/. Please review. https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socke... File net/socket/server_socket.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socke... net/socket/server_socket.cc:18: int ServerSocket::ListenWithAddressAndPort(const std::string& ip_address_string, On 2014/06/11 16:05:50, mmenke wrote: > nit: Use same variable names here as in the header. Done. https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/server_socke... net/socket/server_socket.h:29: // address. On 2014/06/11 16:05:50, mmenke wrote: > The last sentence is confusing - unclear what it's trying to say. Revised in the other CL, https://codereview.chromium.org/348803003/ https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:77: int UnixDomainClientSocket::Connect(const CompletionCallback& callback) { On 2014/06/11 16:05:50, mmenke wrote: > DCHECK(waiting_connect_)? Done. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:79: return ERR_SOCKET_IS_CONNECTED; On 2014/06/11 16:05:50, mmenke wrote: > Should we DCHECK on this instead? (I know we don't do this elsewhere, but the > Chrome pholosophy is to DCHECK and not handle coding bugs, unless they're > security related) Done. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:85: DCHECK(write_callback_.is_null()); On 2014/06/11 16:05:50, mmenke wrote: > Move these up before the IsConnected check? Done. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:279: void UnixDomainClientSocket::DidCompleteWrite() { On 2014/06/11 17:53:27, mmenke wrote: > On 2014/06/11 17:30:28, byungchul wrote: > > On 2014/06/11 16:05:50, mmenke wrote: > > > This function seems to be misnamed (As is DidCompleteRead). Same is true > for > > > tcp_socket_libevent, which this class closely resembles, but since it > doesn't > > > seem to go out of its way to be identical to tcp_socket_libevent... > > > > > > It's a bit unfortunate that we can't easily just reuse all the code for > > > TCPClientSocketLibevent. It's not a huge amount of redundant code, but it > > does > > > mean that any updates to one class won't affect the other. > > > > Per your request, will try to split this change into 2 CLs. One for unix > stream > > socket is the prerequisite for the other of http server refactoring. Will > define > > socket_libevent to be inherited by here and tcp_libevent.cc. Makes sense? > > Yea, makes sense. > > Hrm... Since TCPSocketLibevent doesn't inherit from StreamSocket, I'm not sure > the best way to do this cleanly. Done. Please review https://codereview.chromium.org/348803003/. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.cc:310: NOTREACHED(); On 2014/06/11 16:05:50, mmenke wrote: > Shouldn't be handling this case, should just have DCHECK (TCPSocketLibevent does > the same thing, but believe the consensus now is that we should not handle cases > that should never happen). Same goes for OnFileCanReadWithoutBlocking. Done. https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/296053012/diff/180001/net/socket/unix_domain_... net/socket/unix_domain_client_socket_posix.h:70: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE; On 2014/06/11 16:05:50, mmenke wrote: > These aren't intended to be used by consumers, so shouldn't be public. Done.
Not a very thorough pass today, had less time than expected. :( https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:79: const HttpServerResponseInfo& response) { Maybe rename this to SendResponseHeaders? Could optioanlly also rename HttpServerResponseInfo to HttpServerResponseHeaders https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:118: // callbacks in the call stack return. What callbacks? SocketLibEvent can handle being deleted while calling a callback, no? https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:121: base::Bind(&DeleteConnection, base::Unretained(connection))); DeleteHelper, maybe? Both leak if the task is never run, during shutdown, but don't think we can fix that - if we use base::Owned instead, then if this is called during shutdown, the task will be destroyed instantly. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:131: return; Is there a real need to let this be called for non-existent sockets? Generally, the preference is to crash if something should not happen. Same goes for SetSendBufferSize. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:168: DoReadLoop(connection); Random comment, feel free to ignore: Hrm...we normally allow destruction of HttpServers on close in DidRead. However, we don't allow deletion of the HttpServer in DidAccept, even when it calls DidRead. So if we establish a connection, try to read from it, synchronously get an error, return the error to the Delegate, which then deletes us, looks like we crash. I think the API really needs documentation about when it is and is not safe to delete this. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:195: if (!connection) // It might be closed right before by write error. What's to prevent the HttpServer from being destroyed during the period while the connection still has a pointer to this, and the delete task is pending? https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:202: int HttpServer::DidRead(HttpConnection* connection, int rv) { optional: Suggest renaming this to HandleReadResult. Think that makes it a little clearer what's different between this and OnReadCompleted than the current name. Same suggestion for DidWrite. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.h:65: void SetReceiveBufferSize(int connection_id, int32 size); This seems kinda weird - we don't even tell the Delegate when we receive data, so it can't even call this on connection establishment. Should have have a single size for all connections?
PTAL https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:23: IOBuffer* HttpConnection::ReadIOBuffer::GetUnusedIOBuffer() const { On 2014/06/11 17:53:27, mmenke wrote: > On 2014/06/11 17:30:28, byungchul wrote: > > On 2014/06/11 16:09:04, mmenke wrote: > > > On 2014/06/11 16:05:49, mmenke wrote: > > > > I think this is a very confusing name and concept. My suggestion: Make > > > > ReadIOBuffer inherit from GrowableIOBuffer, and get rid of this function. > > > > > > > > You'd have to rename the current data_ to something else, but I think the > > > usage > > > > pattern would be clearer. I'd suggest just calling it unconsumed_data_. > > Open > > > > to other ideas. > > > > > > Ignore this suggestion - I think we should think about how to make this > > simpler, > > > but I don't think this is the way to go. > > > > Okay, I am re-thinking about how to manage the read buffer. But, please > > understand that this depends on how http server and websocket handle packets. > > They assume read data buffer is continuous. Unless we change this behavior, > it's > > hard to split read buffer in multiple segments to reduce data copy. > > I'm not concerned with reducing data copy for the read buffer, actually, since > we're just reading HTTP requests. I just think the class can be a bit hard to > follow. Thinking more along the lines of improving naming / readability of > ReadIOBuffer, than changing how it basically works. > > My move suggestion below was more aimed at limiting needed buffer size, in the > worst case, rather than improving performance. Changed read buffer similar to GrowableIOBuffer. Couldn't inherit GrowableIOBuffer unfortunately because it is not allowed due to private destructor. Basically, it moves unconsumed data to the start of buffer on DidConsume() call. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:27: int HttpConnection::ReadIOBuffer::GetUnusedCapacity() const { On 2014/06/11 16:05:49, mmenke wrote: > Suggest keeping this as GetRemainingCapacity, for everyone's sanity. Done. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:44: LOG(ERROR) << "Too large read data is pending: capacity=" << GetCapacity() On 2014/06/11 16:05:49, mmenke wrote: > DLOG, to avoid bloating up binaries. Http server closes the connection for this error. Isn't LOG(ERROR) appropriate to show the reason of close? https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:50: int new_capacity = GetCapacity() * kCapacityIncreaseFactor; On 2014/06/11 17:30:28, byungchul wrote: > On 2014/06/11 16:05:49, mmenke wrote: > > Seems a little weird to me that we never make an effort to move unhandled > > requests back to the start of the buffer. I don't see this causing us > problems > > in actual usage, but seems a somewhat weird behavior that we just resize the > > buffer instead of moving all unread data to the start of the buffer, when we > > can. > > Yes, your suggestion would be helpful though this will be called again soon in > practical. Changed to move data on DidConsume() call. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection.cc:147: DCHECK_EQ(total_size_, 0); On 2014/06/11 16:05:49, mmenke wrote: > nit: Expected goes before actual. Done. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:5: #include "net/server/http_connection.h" On 2014/06/11 16:05:49, mmenke wrote: > nit: Line break after main header. Done. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:88: const int read_length = 128; On 2014/06/11 16:05:49, mmenke wrote: > nit: kReadLength (Same goes for the others) Done. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:117: // Reads another data. On 2014/06/11 16:05:49, mmenke wrote: > nit: "Reads more data." Done. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:146: } On 2014/06/11 16:05:49, mmenke wrote: > Need a test where the buffer length is increased when some data has already been > read. Do you mean content itself checking when capacity changed? Done. https://codereview.chromium.org/296053012/diff/180001/net/server/http_connect... net/server/http_connection_unittest.cc:163: const std::string data_2("another data to write"); On 2014/06/11 16:05:50, mmenke wrote: > nit: "more data to write" Done. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:79: const HttpServerResponseInfo& response) { On 2014/07/24 20:50:08, mmenke wrote: > Maybe rename this to SendResponseHeaders? Could optioanlly also rename > HttpServerResponseInfo to HttpServerResponseHeaders It's not just for headers. HttpServerResponseInfo has data. Added a TODO to consider function name change though. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:118: // callbacks in the call stack return. On 2014/07/24 20:50:09, mmenke wrote: > What callbacks? SocketLibEvent can handle being deleted while calling a > callback, no? This is a server. So, it is a common pattern to do something on read operation which may close the connection. It is not only for close on error. This logic is for that normal close by server side. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:121: base::Bind(&DeleteConnection, base::Unretained(connection))); On 2014/07/24 20:50:06, mmenke wrote: > DeleteHelper, maybe? > > Both leak if the task is never run, during shutdown, but don't think we can fix > that - if we use base::Owned instead, then if this is called during shutdown, > the task will be destroyed instantly. Don't know how to use DeleteHelper correctly. Instead, changed to call DeleteSoon. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:131: return; On 2014/07/24 20:50:06, mmenke wrote: > Is there a real need to let this be called for non-existent sockets? Generally, > the preference is to crash if something should not happen. Same goes for > SetSendBufferSize. Done. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:168: DoReadLoop(connection); On 2014/07/24 20:50:09, mmenke wrote: > Random comment, feel free to ignore: Hrm...we normally allow destruction of > HttpServers on close in DidRead. However, we don't allow deletion of the > HttpServer in DidAccept, even when it calls DidRead. So if we establish a > connection, try to read from it, synchronously get an error, return the error to > the Delegate, which then deletes us, looks like we crash. > > I think the API really needs documentation about when it is and is not safe to > delete this. Even onClose() is not safe to destroy http server. Added a comment that in .h file. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:195: if (!connection) // It might be closed right before by write error. On 2014/07/24 20:50:06, mmenke wrote: > What's to prevent the HttpServer from being destroyed during the period while > the connection still has a pointer to this, and the delete task is pending? connection doesn't have the reference for http server. And, added a comment that any of delegate functions are not safe to destroy http server. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.cc:202: int HttpServer::DidRead(HttpConnection* connection, int rv) { On 2014/07/24 20:50:07, mmenke wrote: > optional: Suggest renaming this to HandleReadResult. Think that makes it a > little clearer what's different between this and OnReadCompleted than the > current name. > > Same suggestion for DidWrite. Done. https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/220001/net/server/http_server.... net/server/http_server.h:65: void SetReceiveBufferSize(int connection_id, int32 size); On 2014/07/24 20:50:09, mmenke wrote: > This seems kinda weird - we don't even tell the Delegate when we receive data, > so it can't even call this on connection establishment. Should have have a > single size for all connections? It could be called on AcceptWebSocket().
Doesn't look like I'll have time to do another pass today. I will get back to you tomorrow. How we can get this done and landed pretty promptly, now that we've landed all the prereqs.
I have some ideas on more refactoring after this CL, for example, differentiating http connection and websocket connection because their behaviors are different (client-server vs. simultaneous sending), parse http headers once. On Wed, Aug 6, 2014 at 9:01 AM, <mmenke@chromium.org> wrote: > Doesn't look like I'll have time to do another pass today. I will get > back to > you tomorrow. How we can get this done and landed pretty promptly, now > that > we've landed all the prereqs. > > https://codereview.chromium.org/296053012/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sounds great! And "How" in that last comment should have been "Hope". On 2014/08/06 16:50:29, byungchul wrote: > I have some ideas on more refactoring after this CL, for example, > differentiating http connection and websocket connection because their > behaviors are different (client-server vs. simultaneous sending), parse > http headers once. > > > On Wed, Aug 6, 2014 at 9:01 AM, <mailto:mmenke@chromium.org> wrote: > > > Doesn't look like I'll have time to do another pass today. I will get > > back to > > you tomorrow. How we can get this done and landed pretty promptly, now > > that > > we've landed all the prereqs. > > > > https://codereview.chromium.org/296053012/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Grr...sorry for slowness, been meaning to get to this all week, but just haven't been able to find the time. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.cc:19: data_ = NULL; // base_ owns data_. nit: Not needed. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.cc:42: } nit: There's a general preference in net/ not to use braces in these cases. Same goes for the rest of these 1-line bodies. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:25: // functions for buffer management. It moves unconsumed data to the start of Maybe replace second sentence with "It's a wrapper around GrowableIOBuffer, with more functions for buffer management." https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:41: // More read data is appended. This doesn't actually append more data. Maybe "was appended"? https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:50: int capacity_limit() const { return capacity_limit_; } Really should use consistent terminology here and write buffer (capacity_limit vs "size_limit") https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:51: void set_capacity_limit(int limit) { capacity_limit_ = limit; } nit: limit->capacity_limit https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:72: class PendingWriteIOBuffer : public IOBuffer { I wonder if this really should be an IOBuffer - the multiple string thing makes this behave a lot differently than other IOBuffers, and some of the IOBuffer's description doesn't really apply to PendingWriteIOBuffer. One option would be just to keep a queue of IOBufferWithSizes - the API and code would be pretty much the same, my only concern is whether this really implements the IOBuffer interface. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:84: // Consumes data and changes data() accordingly. Worth mentioning it can't be mroe than GetSizeToWrite(). https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:90: int total_size() const { return total_size_; } I don't think this is used. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:92: int total_size_limit() const { return total_size_limit_; } Is this used anywhere? https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:93: void set_total_size_limit(int limit) { total_size_limit_ = limit; } Suggest renaming total_size_limit_ to max_buffer_size, to make it clear this is not a limit on the upload size (Since we have no callback to say we need more data, it's effectively the upload size limit, too, but based on implementation, it's intended just to be the buffer size limit) https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:103: int total_size_; Is this used for anything other than sanity checks? Think we can get rid of it. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:9: namespace net { nit: Put entire file in anonymous namespace. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:12: protected: nit: Just make it public, don't get anything out of the protected in tests. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:21: HttpConnection::PendingWriteIOBuffer::kDefaultTotalSizeLimit; Suggest just making these public in the base class, so we don't have to be a friend of the test classes. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:23: static scoped_refptr<GrowableIOBuffer> GetBase( I don't think these tests should dig into the implementation details of ReadBuffer wrapping a GrowableIOBuffer. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:30: return write_buffer->pending_data_; Again, think these tests should only test the externally available API. There are other ways to test that things are updated properly. https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.... net/server/http_server.cc:182: if (!connection) // It might be closed right before by write error. On 2014/08/05 07:06:19, byungchul wrote: > On 2014/07/24 20:50:06, mmenke wrote: > > What's to prevent the HttpServer from being destroyed during the period while > > the connection still has a pointer to this, and the delete task is pending? > > connection doesn't have the reference for http server. And, added a comment that > any of delegate functions are not safe to destroy http server. The connection does have a pointer, actually. Or more accurately, its socket does. So here's how things can happen: * HttpServer::DoReadLoop calls connection->socket()->Read. * Owner calls HttpServer::Close on the connection, removing the connection from the list, and posting a task to delete the connection. * Owner deletes the HttpServer. * The socket receives data, before the connection is deleted. It calls the destroyed HttpServer's OnReadCompleted function. * We crash. https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.... net/server/http_server.h:65: void SetReceiveBufferSize(int connection_id, int32 size); On 2014/08/05 07:06:20, byungchul wrote: > On 2014/07/24 20:50:09, mmenke wrote: > > This seems kinda weird - we don't even tell the Delegate when we receive data, > > so it can't even call this on connection establishment. Should have have a > > single size for all connections? > > It could be called on AcceptWebSocket(). I'm fine with adding the function, but I think this CL already adds too much. I'd actually suggest getting rid of these two functions in this CL, and adding them in a followup CL, but I don't feel too strongly about that.
https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.cc:19: data_ = NULL; // base_ owns data_. On 2014/08/08 18:35:43, mmenke wrote: > nit: Not needed. IOBuffer::~IOBuffer deletes data_ if it is not null. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.cc:42: } On 2014/08/08 18:35:43, mmenke wrote: > nit: There's a general preference in net/ not to use braces in these cases. > Same goes for the rest of these 1-line bodies. Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:25: // functions for buffer management. It moves unconsumed data to the start of On 2014/08/08 18:35:43, mmenke wrote: > Maybe replace second sentence with "It's a wrapper around GrowableIOBuffer, with > more functions for buffer management." Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:41: // More read data is appended. On 2014/08/08 18:35:43, mmenke wrote: > This doesn't actually append more data. Maybe "was appended"? Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:50: int capacity_limit() const { return capacity_limit_; } On 2014/08/08 18:35:43, mmenke wrote: > Really should use consistent terminology here and write buffer (capacity_limit > vs "size_limit") Changed to max_buffer_size. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:51: void set_capacity_limit(int limit) { capacity_limit_ = limit; } On 2014/08/08 18:35:43, mmenke wrote: > nit: limit->capacity_limit Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:72: class PendingWriteIOBuffer : public IOBuffer { On 2014/08/08 18:35:43, mmenke wrote: > I wonder if this really should be an IOBuffer - the multiple string thing makes > this behave a lot differently than other IOBuffers, and some of the IOBuffer's > description doesn't really apply to PendingWriteIOBuffer. > > One option would be just to keep a queue of IOBufferWithSizes - the API and code > would be pretty much the same, my only concern is whether this really implements > the IOBuffer interface. IOBuffer has only data() and doesn't imply anything about size. So, yes it implements IOBuffer correctly. Your suggestions would be fine except when the buffer was not written fully. The caller needs to handle unwritten data (moving data to the start of buffer) which break information hiding or modular programming. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:84: // Consumes data and changes data() accordingly. On 2014/08/08 18:35:43, mmenke wrote: > Worth mentioning it can't be mroe than GetSizeToWrite(). Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:90: int total_size() const { return total_size_; } On 2014/08/08 18:35:43, mmenke wrote: > I don't think this is used. Used in unittests. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:92: int total_size_limit() const { return total_size_limit_; } On 2014/08/08 18:35:43, mmenke wrote: > Is this used anywhere? in unittests. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:93: void set_total_size_limit(int limit) { total_size_limit_ = limit; } On 2014/08/08 18:35:43, mmenke wrote: > Suggest renaming total_size_limit_ to max_buffer_size, to make it clear this is > not a limit on the upload size (Since we have no callback to say we need more > data, it's effectively the upload size limit, too, but based on implementation, > it's intended just to be the buffer size limit) Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection.h:103: int total_size_; On 2014/08/08 18:35:43, mmenke wrote: > Is this used for anything other than sanity checks? Think we can get rid of it. Used in Append() not to queue more than capacity. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:9: namespace net { On 2014/08/08 18:35:44, mmenke wrote: > nit: Put entire file in anonymous namespace. Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:12: protected: On 2014/08/08 18:35:43, mmenke wrote: > nit: Just make it public, don't get anything out of the protected in tests. Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:21: HttpConnection::PendingWriteIOBuffer::kDefaultTotalSizeLimit; On 2014/08/08 18:35:44, mmenke wrote: > Suggest just making these public in the base class, so we don't have to be a > friend of the test classes. Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:23: static scoped_refptr<GrowableIOBuffer> GetBase( On 2014/08/08 18:35:44, mmenke wrote: > I don't think these tests should dig into the implementation details of > ReadBuffer wrapping a GrowableIOBuffer. Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_connect... net/server/http_connection_unittest.cc:30: return write_buffer->pending_data_; On 2014/08/08 18:35:43, mmenke wrote: > Again, think these tests should only test the externally available API. There > are other ways to test that things are updated properly. Done. https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.... net/server/http_server.cc:182: if (!connection) // It might be closed right before by write error. On 2014/08/08 18:35:44, mmenke wrote: > On 2014/08/05 07:06:19, byungchul wrote: > > On 2014/07/24 20:50:06, mmenke wrote: > > > What's to prevent the HttpServer from being destroyed during the period > while > > > the connection still has a pointer to this, and the delete task is pending? > > > > connection doesn't have the reference for http server. And, added a comment > that > > any of delegate functions are not safe to destroy http server. > > The connection does have a pointer, actually. Or more accurately, its socket > does. So here's how things can happen: > > * HttpServer::DoReadLoop calls connection->socket()->Read. > * Owner calls HttpServer::Close on the connection, removing the connection from > the list, and posting a task to delete the connection. > * Owner deletes the HttpServer. > * The socket receives data, before the connection is deleted. It calls the > destroyed HttpServer's OnReadCompleted function. > * We crash. Changed to weak ptr. https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/330001/net/server/http_server.... net/server/http_server.h:65: void SetReceiveBufferSize(int connection_id, int32 size); On 2014/08/08 18:35:44, mmenke wrote: > On 2014/08/05 07:06:20, byungchul wrote: > > On 2014/07/24 20:50:09, mmenke wrote: > > > This seems kinda weird - we don't even tell the Delegate when we receive > data, > > > so it can't even call this on connection establishment. Should have have a > > > single size for all connections? > > > > It could be called on AcceptWebSocket(). > > I'm fine with adding the function, but I think this CL already adds too much. > I'd actually suggest getting rid of these two functions in this CL, and adding > them in a followup CL, but I don't feel too strongly about that. SetSendBufferSize() is used by devtools.
Hrm....Maybe we will be able to land this by Friday... Not noticing a whole lot. I'll start a pass today, but may not get back to you until tomorrow morning, depending on how many comments I have.
https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.cc:35: << ", read=" << GetSize(); include base/logging.h https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.cc:77: && GetCapacity() > previous_size * kCapacityIncreaseFactor) { nit: put && on previous line https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.cc:150: scoped_ptr<StreamSocket> socket) I assume this doesn't quite fit on one line? https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.h:12: #include "base/memory/scoped_ptr.h" Should include scoped_refptr https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.h:72: class PendingWriteIOBuffer : public IOBuffer { optional: "QueuedWriteIOBuffer" may be clearer, fine with the current name, though. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:12: class HttpConnectionTest : public testing::Test { This test fixture doesn't really get us anything, can just get rid of it, and move the functions up into the anonymous namespace. Then just replace TEST_F with TEST. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:14: static void CheckCapacity(scoped_refptr<HttpConnection::ReadIOBuffer> buffer, include ref_counted https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:20: static std::string GetTestString(int size) { include <string> https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:38: EXPECT_EQ(0, buffer->GetSize()); Should we write kInitialBufSize bytes to the buffer, and make sure they're still there after we update capacity? And then write 128 bytes to the end, to make sure the capacity was really updated? https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:71: buffer->GetCapacity()); Same goes for this - should we make sure that we keep the old bytes? https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:110: - kReadLength + kConsumedLength, Put binary operators on previous line (Goes for this entire file) https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:191: buffer->total_size()); Should have a partial read, with an EXPECT_EQ for the remaining unread data. https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.cc:110: // callbacks in the call stack return. This seems really weird - this means we can still read from a "closed" connection, and pass data along to the delegate after the consumer closed a connection, and we tell the delegate the connection has been closed... Or am I missing something? https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.h:29: // functions are not safe for http server destruction. nit: Maybe "It's not safe to destroy the HttpServer in any of these callbacks." https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.h:52: // TODO(byungchul): Consider to replace function name with SendResponseInfo nit: "to replace" -> "replacing" https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.h:67: void SetSendBufferSize(int connection_id, int32 size); Should include the header for int32 (Not sure what header it is) https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.cc File net/server/web_socket.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.c... net/server/web_socket.cc:52: < static_cast<int>(*pos + kWebSocketHandshakeBodyLen)) nit: Put operators on previous line. https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.h File net/server/web_socket.h (right): https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.h... net/server/web_socket.h:47: explicit WebSocket(HttpServer* server, HttpConnection* connection); nit: Explicit no longer needed.
https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... File net/server/http_connection.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.cc:35: << ", read=" << GetSize(); On 2014/08/14 16:36:02, mmenke wrote: > include base/logging.h Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.cc:77: && GetCapacity() > previous_size * kCapacityIncreaseFactor) { On 2014/08/14 16:36:02, mmenke wrote: > nit: put && on previous line Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.cc:150: scoped_ptr<StreamSocket> socket) On 2014/08/14 16:36:02, mmenke wrote: > I assume this doesn't quite fit on one line? Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... File net/server/http_connection.h (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.h:12: #include "base/memory/scoped_ptr.h" On 2014/08/14 16:36:02, mmenke wrote: > Should include scoped_refptr Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection.h:72: class PendingWriteIOBuffer : public IOBuffer { On 2014/08/14 16:36:02, mmenke wrote: > optional: "QueuedWriteIOBuffer" may be clearer, fine with the current name, > though. Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:12: class HttpConnectionTest : public testing::Test { On 2014/08/14 16:36:02, mmenke wrote: > This test fixture doesn't really get us anything, can just get rid of it, and > move the functions up into the anonymous namespace. Then just replace TEST_F > with TEST. Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:14: static void CheckCapacity(scoped_refptr<HttpConnection::ReadIOBuffer> buffer, On 2014/08/14 16:36:02, mmenke wrote: > include ref_counted Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:20: static std::string GetTestString(int size) { On 2014/08/14 16:36:02, mmenke wrote: > include <string> Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:38: EXPECT_EQ(0, buffer->GetSize()); On 2014/08/14 16:36:02, mmenke wrote: > Should we write kInitialBufSize bytes to the buffer, and make sure they're still > there after we update capacity? And then write 128 bytes to the end, to make > sure the capacity was really updated? Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:71: buffer->GetCapacity()); On 2014/08/14 16:36:02, mmenke wrote: > Same goes for this - should we make sure that we keep the old bytes? Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:110: - kReadLength + kConsumedLength, On 2014/08/14 16:36:02, mmenke wrote: > Put binary operators on previous line (Goes for this entire file) Done. I knew the style but it was more readable for me. https://codereview.chromium.org/296053012/diff/370001/net/server/http_connect... net/server/http_connection_unittest.cc:191: buffer->total_size()); On 2014/08/14 16:36:02, mmenke wrote: > Should have a partial read, with an EXPECT_EQ for the remaining unread data. Did you mean partial consumption on ReadIOBuffer? It is done in ReadIOBuffer_DidRead_DidConsume test. https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.cc:110: // callbacks in the call stack return. On 2014/08/14 16:36:02, mmenke wrote: > This seems really weird - this means we can still read from a "closed" > connection, and pass data along to the delegate after the consumer closed a > connection, and we tell the delegate the connection has been closed... Or am I > missing something? Changed to do entire Close() in next run loop. https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.h:29: // functions are not safe for http server destruction. On 2014/08/14 16:36:03, mmenke wrote: > nit: Maybe "It's not safe to destroy the HttpServer in any of these callbacks." Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.h:52: // TODO(byungchul): Consider to replace function name with SendResponseInfo On 2014/08/14 16:36:02, mmenke wrote: > nit: "to replace" -> "replacing" Done. https://codereview.chromium.org/296053012/diff/370001/net/server/http_server.... net/server/http_server.h:67: void SetSendBufferSize(int connection_id, int32 size); On 2014/08/14 16:36:03, mmenke wrote: > Should include the header for int32 (Not sure what header it is) Done. Defined in base/basictypes.h. https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.cc File net/server/web_socket.cc (right): https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.c... net/server/web_socket.cc:52: < static_cast<int>(*pos + kWebSocketHandshakeBodyLen)) On 2014/08/14 16:36:03, mmenke wrote: > nit: Put operators on previous line. Done. https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.h File net/server/web_socket.h (right): https://codereview.chromium.org/296053012/diff/370001/net/server/web_socket.h... net/server/web_socket.h:47: explicit WebSocket(HttpServer* server, HttpConnection* connection); On 2014/08/14 16:36:03, mmenke wrote: > nit: Explicit no longer needed. Done.
https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... net/server/http_connection_unittest.cc:48: // Write arbitrara data up to kInitialBufSize. nit: arbitrary https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... net/server/http_connection_unittest.cc:49: std::string data_read(HttpConnection::ReadIOBuffer::kInitialBufSize, 'd'); Suggest something more interesting than a single character, since all it tests is we have some random subset of the original buffer. Goes for ReadIOBuffer_IncreaseCapacity_WithData as well. https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... net/server/http_connection_unittest.cc:262: buffer->total_size()); On 2014/08/14 18:44:03, byungchul wrote: > On 2014/08/14 16:36:02, mmenke wrote: > > Should have a partial read, with an EXPECT_EQ for the remaining unread data. > > Did you mean partial consumption on ReadIOBuffer? It is done in > ReadIOBuffer_DidRead_DidConsume test. I mean partial consumption of a write buffer. You never check that in the partial write case, the apparent contents of the buffer (buffer->data()) are updated properly. https://codereview.chromium.org/296053012/diff/410001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/410001/net/server/http_server.... net/server/http_server.cc:107: weak_ptr_factory_.GetWeakPtr(), connection_id)); This is my remaining concern - this is really weird. This is a public function, that the consumer can call directly. So we can still send it some data for a given connection, after it calls Close() on it. I think we really need a reasonable story here. One option is to check if a connection is closed in any call that may nest down to this level. Gets ugly, and hard to be sure we get them all, but it works. Or we could delete the connection here, and keep a weak pointer to it and check that - I prefer that approach. Open to other ideas. Suppose we could also make it an error to close a connection during a callback, but that seems too draconian.
https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... File net/server/http_connection_unittest.cc (right): https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... net/server/http_connection_unittest.cc:48: // Write arbitrara data up to kInitialBufSize. On 2014/08/15 14:23:16, mmenke wrote: > nit: arbitrary Done. https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... net/server/http_connection_unittest.cc:49: std::string data_read(HttpConnection::ReadIOBuffer::kInitialBufSize, 'd'); On 2014/08/15 14:23:16, mmenke wrote: > Suggest something more interesting than a single character, since all it tests > is we have some random subset of the original buffer. > > Goes for ReadIOBuffer_IncreaseCapacity_WithData as well. Done. https://codereview.chromium.org/296053012/diff/410001/net/server/http_connect... net/server/http_connection_unittest.cc:262: buffer->total_size()); On 2014/08/15 14:23:16, mmenke wrote: > On 2014/08/14 18:44:03, byungchul wrote: > > On 2014/08/14 16:36:02, mmenke wrote: > > > Should have a partial read, with an EXPECT_EQ for the remaining unread data. > > > > Did you mean partial consumption on ReadIOBuffer? It is done in > > ReadIOBuffer_DidRead_DidConsume test. > > I mean partial consumption of a write buffer. You never check that in the > partial write case, the apparent contents of the buffer (buffer->data()) are > updated properly. This case does that. It writes kData and kData2, and consumes kData - 1 at first. So, it reads partially for first data. Added checking data contents. https://codereview.chromium.org/296053012/diff/410001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/410001/net/server/http_server.... net/server/http_server.cc:107: weak_ptr_factory_.GetWeakPtr(), connection_id)); On 2014/08/15 14:23:16, mmenke wrote: > This is my remaining concern - this is really weird. This is a public function, > that the consumer can call directly. > > So we can still send it some data for a given connection, after it calls Close() > on it. > > I think we really need a reasonable story here. One option is to check if a > connection is closed in any call that may nest down to this level. Gets ugly, > and hard to be sure we get them all, but it works. > > Or we could delete the connection here, and keep a weak pointer to it and check > that - I prefer that approach. Open to other ideas. Suppose we could also make > it an error to close a connection during a callback, but that seems too > draconian. Took the first approach because delegate_ is private and easy to track all callbacks.
LGTM https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.... net/server/http_server.cc:215: } nit: Don't use braces for ifs with single line bodies (Same goes the other checks you added) https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.... net/server/http_server.h:99: bool IsConnectionClosed(HttpConnection* connection); This function is confusingly named, since it has nothing to do with whether the socket is actually closed or not, just whether Close() was called. Maybe HasClosedConnection, with a comment about that?
https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.... net/server/http_server.cc:215: } On 2014/08/15 17:53:02, mmenke wrote: > nit: Don't use braces for ifs with single line bodies (Same goes the other > checks you added) Done. https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/296053012/diff/430001/net/server/http_server.... net/server/http_server.h:99: bool IsConnectionClosed(HttpConnection* connection); On 2014/08/15 17:53:02, mmenke wrote: > This function is confusingly named, since it has nothing to do with whether the > socket is actually closed or not, just whether Close() was called. > > Maybe HasClosedConnection, with a comment about that? Done.
devtools lgtm, but you need no move the socket factory out of devtools_http_handler. https://codereview.chromium.org/296053012/diff/490001/content/public/browser/... File content/public/browser/devtools_http_handler.h (right): https://codereview.chromium.org/296053012/diff/490001/content/public/browser/... content/public/browser/devtools_http_handler.h:33: class CONTENT_EXPORT ServerSocketFactory { - content API suggests that all interfaces are defined in their own classes - this does not belong neither to the devtools nor to content, could you expose it in next to the net::ServerSocket?
https://codereview.chromium.org/296053012/diff/490001/content/public/browser/... File content/public/browser/devtools_http_handler.h (right): https://codereview.chromium.org/296053012/diff/490001/content/public/browser/... content/public/browser/devtools_http_handler.h:33: class CONTENT_EXPORT ServerSocketFactory { On 2014/08/18 15:18:00, pfeldman wrote: > - content API suggests that all interfaces are defined in their own classes > - this does not belong neither to the devtools nor to content, could you expose > it in next to the net::ServerSocket? This factory is used only by devtools to give delay between DevToolsHttpHandler::Start() and server socket creation, actullay to run them in different thread. Aren't many delegate defined in its delegated class, for example, https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... ?
https://codereview.chromium.org/296053012/diff/490001/content/public/browser/... File content/public/browser/devtools_http_handler.h (right): https://codereview.chromium.org/296053012/diff/490001/content/public/browser/... content/public/browser/devtools_http_handler.h:33: class CONTENT_EXPORT ServerSocketFactory { On 2014/08/18 15:18:00, pfeldman wrote: > - content API suggests that all interfaces are defined in their own classes > - this does not belong neither to the devtools nor to content, could you expose > it in next to the net::ServerSocket? This doesn't really seem to belong in net/. It's a wrapper around net's server socket constructors specifically intended for use by DevTools, and does not seem generally useful.
Unfortunately, I'm not a great android OWNER for this. Normally I'd say mnaganov but he appears to be OOO. If pfeldman or mmenke have looked at them then I'd say lg
@Yaron: I looked at the Android parts, they look good. @byungchul: TBR mnaganov@ for them should be fine.
chromecast/ LGTM. I patched this into https://codereview.chromium.org/490603002/ and confirmed Android side still builds.
The CQ bit was checked by byungchul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/byungchul@chromium.org/296053012/510001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rubber stamp LGTM
The CQ bit was checked by byungchul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/byungchul@chromium.org/296053012/510001
Message was sent while issue was closed.
Committed patchset #28 (510001) as 291447
Message was sent while issue was closed.
A revert of this CL (patchset #29) has been created in https://codereview.chromium.org/497223003/ by estade@chromium.org. The reason for reverting is: looks like it caused net_unittests failures: http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20(2)/builds/.... |
