Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
690903002:
Remove timing limitation of SetOption invocation for PPAPI sockets. (Closed)
|
Created:
6 years, 1 month ago by hidehiko Modified:
6 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, jam, raymes+watch_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, mmenke, ihf+watch_chromium.org, Nike, wtc, Ryan Hamilton, jar (doing other things), Ryan Sleevi, Sergey Ulanov, rvargas (doing something else), Scott Hess - ex-Googler Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove timing limitation of SetOption invocation for PPAPI sockets.
Currently PPAPI has timing limitation for sockets' SetOption.
NODELAY, and BROADCAST need to be before Connect() or Bind(),
while RCVBUF_SIZE and SNFBUF_SIZE need to be after it.
This CL removes such a limitation.
Along with the change, pepper_udp_socket_message_filter starts to use UDPSocket instead of UDPServerSocket, so that the implementation direction gets closer to TCP message filter a little bit.
BUG=425563, 420697
TEST=Ran trybots.
Committed: https://crrev.com/fd305c12b3fe8b4d2496f67aed089ff703f19b6d
Cr-Commit-Position: refs/heads/master@{#307867}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 25
Patch Set 5 : Rebase #Patch Set 6 : #
Total comments: 6
Patch Set 7 : Rebase #Patch Set 8 : #Patch Set 9 : Rebase #Patch Set 10 : #
Total comments: 8
Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Total comments: 4
Patch Set 13 : Rebase and fixed implementation. #
Total comments: 22
Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Messages
Total messages: 71 (17 generated)
hidehiko@chromium.org changed reviewers: + dmichael@chromium.org, mmenke@chromium.org, sergeyu@chromium.org
Dave, could you review this CL from PPAPI's point of view? Matt, could you review net/... codes as an OWNER? Sergey, could you review content/browser/renderer_host/p2p/socket_host_udp_unittest.cc as an OWNER? I'd appreciate your help in advance, - hidehiko
I'm not terribly familiar with the UDP code. Maybe someone who's been working on QUIC, like rch, or the person who's been doing the MDNS reviews (ttuttle, I think?) https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:323: return OK; Sockets can be disconnected and then reconnected, so this is rather weird - setting buffer size, opening, closing, and then reopening keeps the size, but opening, setting size, closing, and then reopening does not. Same goes for pretty much all the changes you've made. Reusing a socket like that is rather weird, but since we allow it, should have consistent behaviors when someone does it.
On 2014/10/30 14:40:38, mmenke wrote: > I'm not terribly familiar with the UDP code. Maybe someone who's been working > on QUIC, like rch, or the person who's been doing the MDNS reviews (ttuttle, I > think?) Thank you for review, and suggesting reviewers. I'll add them once I've addressed your review comments. Before that, Dave, is it expected usage from PPAPI point of view to reuse a socket after Close() invocation? (I.e., should I support socket reusing case in PPAPI layer, too? or should I assert, return an error or do something?) Thanks! - hidehiko > > https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... > File net/udp/udp_socket_libevent.cc (right): > > https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... > net/udp/udp_socket_libevent.cc:323: return OK; > Sockets can be disconnected and then reconnected, so this is rather weird - > setting buffer size, opening, closing, and then reopening keeps the size, but > opening, setting size, closing, and then reopening does not. > > Same goes for pretty much all the changes you've made. > > Reusing a socket like that is rather weird, but since we allow it, should have > consistent behaviors when someone does it.
dmichael@chromium.org changed reviewers: + bbudge@chromium.org
FYI, bbudge is going to look, since he's also working on the socket APIs right now.
Patchset #4 (id:60001) has been deleted
Just FYI, moving myself from the reviewer to the CC list, to clean up my pending review list.
hidehiko@chromium.org changed reviewers: + rch@chromium.org - bbudge@chromium.org, mmenke@chromium.org
Ryan, could you kindly review net/... part of this CL as an OWNER? I'd appreciate your help in advance, - hidehiko
https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:323: return OK; On 2014/10/30 14:40:38, mmenke wrote: > Sockets can be disconnected and then reconnected, so this is rather weird - > setting buffer size, opening, closing, and then reopening keeps the size, but > opening, setting size, closing, and then reopening does not. > > Same goes for pretty much all the changes you've made. > > Reusing a socket like that is rather weird, but since we allow it, should have > consistent behaviors when someone does it. oops, sorry, I forgot to reply this comment. I made a change for net/... code. I kept PPAPI related code as is, assuming reusing a closed socket is unexpected use case.
hidehiko@chromium.org changed reviewers: + bbudge@chromium.org
+bbudge@. (I should have noticed he's not in the R= list...) Could you take a look? Ryan, friendly ping? Thank you for review in advance, - hidehiko On 2014/11/03 13:34:37, hidehiko wrote: > https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... > File net/udp/udp_socket_libevent.cc (right): > > https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev... > net/udp/udp_socket_libevent.cc:323: return OK; > On 2014/10/30 14:40:38, mmenke wrote: > > Sockets can be disconnected and then reconnected, so this is rather weird - > > setting buffer size, opening, closing, and then reopening keeps the size, but > > opening, setting size, closing, and then reopening does not. > > > > Same goes for pretty much all the changes you've made. > > > > Reusing a socket like that is rather weird, but since we allow it, should have > > consistent behaviors when someone does it. > > oops, sorry, I forgot to reply this comment. I made a change for net/... code. I > kept PPAPI related code as is, assuming reusing a closed socket is unexpected > use case.
rch@chromium.org changed reviewers: + jar@chromium.org
Sorry to redirect you again. I'm going to actually hand this off to jar who has mucked around much more with this code. I just discussed it with him, and I think you won't get redirected again.
Can you better motivate why this is a valuable change, rather than merely correcting the code of the callers to comply with existing (and generally expected/standard(?)) sockets interfaces? Although the plan of supporting premature calls (that are enacted later, when socket()s allow), it does not make sense to make calls "late" that must have been made earlier to have any impact (e.g., REUSE_ADDR, etc.). In addition, the semantics, and possible return values of many methods seem to be changed by this CL, and whole new semantics established (to be fair... only in new/strange cases... but programmers need to understand what error returns are possible from methods... and this seems to grow that list, adding to complexity for all users of these methods :-/ ). There are some comments below... but they came from a fast read, and it would not be sufficient to resolve them without much better motivation (and additional reviews). Apologies in advance if my fast-scan misread things. https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:792: SendConnectError(context, pp_result); What are the list of previously expectable error conditions from DoBind? Haven't you expanded the list now? https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:797: // because in the private or v1.0 API, connect must be the first operation. This comment appears to be broken by this change. https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:198: // Receive/Send buffer sizes which is set before connect(). This code actually sets this value whether called before, or after connect(). I think the comment should be something like: "Locally cached value of buffer sizes, if set to a non-default value." https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (left): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:267: socket->AllowAddressReuse(); For better or worse... it appears that the old code did not monitor return/error values. https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:291: SendBindError(context, pp_result); It is valid to SendBindError an error which may previously never have been seen by that function? https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:344: if (is_connected()) { What are the semantics for setting AllowAddressRuse *after* connecting? Is this just an invitation for an effective no-op, with no programmer warning? https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:364: int UDPSocketLibevent::AllowBroadcast() { Why don't you use a single setter method, taking a boolean, rather than having two nearly identical functions, with mostly duplicated code? Same question for (Dis)AllowAddressReuse(). https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:251: int SetAddressReuseInternal(bool value); nit: use better variable name than |value|. Perhaps |reuse_address|, and in the next line |broadcast|. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:503: } Note that you have changed the semantics here, as now an error falls through, with hopes of being acted upon later. Should this return an error when is_connected() and the internal method fails? https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:215: // Applies |socket_options_| to |socket_|. Should be called before What does this "Should...." comment mean now? https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:219: int SetBroadcastInternal(bool value); nit: please use a better variable name in the above two lines. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:238: // Buffer size set before Bind(). This should be applied Rather than the passive voice "This should..." Please use active voice "This is automatically applied when SetSocketOptions() is called."
https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:503: } On 2014/11/04 22:51:09, jar wrote: > Note that you have changed the semantics here, as now an error falls through, > with hopes of being acted upon later. > > Should this return an error when is_connected() and the internal method fails? <doh>... My mistake... please ignore this comment.
Patchset #6 (id:120001) has been deleted
Thank you for review! PATL. The main goal of this CL is fixing crbug.com/425563. Could you look at it? As for, e.g., SO_REUSEADDR after bind(), it actually has an effect. So, this change is valuable. Also, this CL changes the behavior of the cases that are currently treated as errors. So I hope this has not that big side effect. In addition, this CL changes the socket behavior closer to one on the linux for example, I believe added edge cases are acceptable. https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:792: SendConnectError(context, pp_result); On 2014/11/04 22:51:08, jar wrote: > What are the list of previously expectable error conditions from DoBind? Haven't > you expanded the list now? IIUC, it is not managed, at least not-documented. Dave, Bill, any thoughts? According to man, all errors possibly returned from setsockopt except ENOPROTOOPT can already be returned from this function (with converting it to the PP_ERROR). We do not expect ENOPROTOOPT, because level/optname of setsockopt is fixed in current implementation. Also, practically, at least on linux, setting options always succeeds, so it seems ok. Though, I'm not an expert of other platforms. Do you know if it may fail, like on Windows? https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:797: // because in the private or v1.0 API, connect must be the first operation. On 2014/11/04 22:51:09, jar wrote: > This comment appears to be broken by this change. I think this is still correct comment? Could you kindly explain what is broken? https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:198: // Receive/Send buffer sizes which is set before connect(). On 2014/11/04 22:51:09, jar wrote: > This code actually sets this value whether called before, or after connect(). > > I think the comment should be something like: > > "Locally cached value of buffer sizes, if set to a non-default value." As we do not take care about "default" value here, I just said: "Locally cached value of buffer sizes." Please note that, this is actually used only "before" connect() in *this layer*, unlike net/... changes I made in this CL. https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (left): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:267: socket->AllowAddressReuse(); On 2014/11/04 22:51:09, jar wrote: > For better or worse... it appears that the old code did not monitor return/error > values. Yes... IMHO, we should handle the error, rather than just ignoring it... Any thoughts? https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/80001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:291: SendBindError(context, pp_result); On 2014/11/04 22:51:09, jar wrote: > It is valid to SendBindError an error which may previously never have been seen > by that function? I think it is ok, as this function already returns an error on, like GetLocalAddress failure, for example. Dave, Bill, any comments? https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:344: if (is_connected()) { On 2014/11/04 22:51:09, jar wrote: > What are the semantics for setting AllowAddressRuse *after* connecting? > > Is this just an invitation for an effective no-op, with no programmer warning? It affects the behavior of other socket's for the same address. I think following example illustrates the behavior. Let be two processes, and each has a socket, s1 and s2. case 1) bind(s1, 'localhost', 12345) bind(s2, 'localhost', 12345) <- fails due to address conflicting. case 2) bind(s1, 'localhost', 12345) setsockopt(s2, SO_REUSEADDR, 1) bind(s2, 'localhost', 12345) <- fails due to address conflicting. case 3) setsockopt(s1, SO_REUSEADDR, 1) bind(s1, 'localhost', 12345) setsockopt(s2, SO_REUSEADDR, 1) bind(s2, 'localhost', 12345) <- success case 4) bind(s1, 'localhost', 12345) setsockopt(s1, SO_REUSEADDR, 1) setsockopt(s2, SO_REUSEADDR, 1) bind(s2, 'localhost', 12345) <- success, too! So, setting SO_REUSEADDR after bind() has effect, I think. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:364: int UDPSocketLibevent::AllowBroadcast() { On 2014/11/04 22:51:09, jar wrote: > Why don't you use a single setter method, taking a boolean, rather than having > two nearly identical functions, with mostly duplicated code? > > Same question for (Dis)AllowAddressReuse(). I just kept the current interface, as UDPSocket{Libevent,Win}::Allow{Broadcast,AddressReuse} are used from several places outside from net/... and ppapi/... If unified version is better, I'm happy to work on unifying (Dis)Allow... methods, but I'd like to do it in a separate CL as a refactoring, in order to focus on fixing PPAPI in this CL. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:251: int SetAddressReuseInternal(bool value); On 2014/11/04 22:51:09, jar wrote: > nit: use better variable name than |value|. > > Perhaps |reuse_address|, and in the next line |broadcast|. Done. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:503: } On 2014/11/05 01:18:31, jar wrote: > On 2014/11/04 22:51:09, jar wrote: > > Note that you have changed the semantics here, as now an error falls through, > > with hopes of being acted upon later. > > > > Should this return an error when is_connected() and the internal method fails? > > <doh>... My mistake... please ignore this comment. Acknowledged. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:215: // Applies |socket_options_| to |socket_|. Should be called before On 2014/11/04 22:51:09, jar wrote: > What does this "Should...." comment mean now? Maybe we should say DoBind rather than Bind. To be honest, I'm not sure its meaning in the original context, but what Bind() does are: - create an socket. - set options which are set before Bind() invocation. - Run DoBind() for actual bind operation. We need to call SetSocketOptions between socket creation and DoBind(). https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:219: int SetBroadcastInternal(bool value); On 2014/11/04 22:51:09, jar wrote: > nit: please use a better variable name in the above two lines. Done. https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:238: // Buffer size set before Bind(). This should be applied On 2014/11/04 22:51:09, jar wrote: > Rather than the passive voice "This should..." Please use active voice "This is > automatically applied when SetSocketOptions() is called." Done.
Some nits, and a general comment: I'm worried about this behavior change. I don't think our test coverage is that great in browser_tests, since some platforms are disabled. You might want to enable as many of the tests as you can and do a try run to make this really does work on all platforms. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/ppapi/... Another worry is that it might break some working plugins. It depends on whether SetOption can ever return an error. If so, then a plugin that failed when calling SetOption but then succeeded when calling Bind or Connect, would now fail when calling Bind or Connect. To be safe, you could version the APIs. I realize that's a pain. WDYT? https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:66: enum SocketOptions { nit: Make this consistent with UDP name (SocketOption). https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:194: // Bitwise-or of SocketOption flags. This stores the state about whether nit: doesn't match enum name above. https://codereview.chromium.org/690903002/diff/140001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/690903002/diff/140001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:290: // All ADDRESS_REUSE, BROADCAST, SEND_BUFFER_SIZE and RECV_BUFFER_SIZE can s/All // or s/All /All of/
rch@chromium.org changed reviewers: - rch@chromium.org
Thank you for review. PTAL.
As for browser_tests, which platform do you mean, Bill?
It seems the {TCP,UDP}Socket's SetOption tests run on same platforms where other
tests run:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/ppapi/...
and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/ppapi/...
As for versionize, there are couple of way to do.
1) Add a new version of interface, like TPC_Socket_1_2. This seems a bit
painful, because we do NOT change the interface in this case, and generator.py
does not support such a situation.
2) Add new flags to Options. Such as:
TCP_SOCKET_OPTION_NO_DELAY_NEW or something (better names are welcome), and
remove timing limitation only for those new flags.
3) Rename the existing flags to, like, TCP_SOCKET_OPTION_NO_DELAY_DEPRECATED or
something, and add yet another new TCP_SOCKET_OPTION_NO_DELAY with new value.
Remove timing limitation only for new flags.
Which approach do you prefere, Bill? I'll make a change upon your preference.
Also, IMHO, it is ok to check Bind()/Connect() invocation at ppapi/proxy/...
layer only for simplicity. WDYT?
https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):
https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:66: enum
SocketOptions {
On 2014/11/06 01:27:55, bbudge wrote:
> nit: Make this consistent with UDP name (SocketOption).
Done.
https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:194: //
Bitwise-or of SocketOption flags. This stores the state about whether
On 2014/11/06 01:27:55, bbudge wrote:
> nit: doesn't match enum name above.
Acknowledged.
https://codereview.chromium.org/690903002/diff/140001/ppapi/tests/test_udp_so...
File ppapi/tests/test_udp_socket.cc (right):
https://codereview.chromium.org/690903002/diff/140001/ppapi/tests/test_udp_so...
ppapi/tests/test_udp_socket.cc:290: // All ADDRESS_REUSE, BROADCAST,
SEND_BUFFER_SIZE and RECV_BUFFER_SIZE can
On 2014/11/06 01:27:55, bbudge wrote:
> s/All // or
> s/All /All of/
Done.
jar@chromium.org changed reviewers: + rvargas@chromium.org, shess@chromium.org - jar@chromium.org
I too continue to be nervous about changing semantics, and vastly prefer avoiding such a change whenever alternatives exist. I'm going to punt this over to Ricardo (who is a Windows expert as well as a Net reviewer), and would probably like to get commentary from a Mac reviewer (but Scott appears to be on leave... so it would be nice to hunt down a replacement).
On 2014/11/06 14:13:22, hidehiko wrote:
> Thank you for review. PTAL.
>
> As for browser_tests, which platform do you mean, Bill?
> It seems the {TCP,UDP}Socket's SetOption tests run on same platforms where
other
> tests run:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/ppapi/...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/ppapi/...
>
> As for versionize, there are couple of way to do.
> 1) Add a new version of interface, like TPC_Socket_1_2. This seems a bit
> painful, because we do NOT change the interface in this case, and generator.py
> does not support such a situation.
Have you tried it? It should work. If it doesn't, that's a bug in the IDL
generator.
>
> 2) Add new flags to Options. Such as:
> TCP_SOCKET_OPTION_NO_DELAY_NEW or something (better names are welcome), and
> remove timing limitation only for those new flags.
>
> 3) Rename the existing flags to, like, TCP_SOCKET_OPTION_NO_DELAY_DEPRECATED
or
> something, and add yet another new TCP_SOCKET_OPTION_NO_DELAY with new value.
> Remove timing limitation only for new flags.
>
> Which approach do you prefere, Bill? I'll make a change upon your preference.
> Also, IMHO, it is ok to check Bind()/Connect() invocation at ppapi/proxy/...
> layer only for simplicity. WDYT?
>
>
https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere...
> File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
> (right):
>
>
https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere...
> content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:66:
enum
> SocketOptions {
> On 2014/11/06 01:27:55, bbudge wrote:
> > nit: Make this consistent with UDP name (SocketOption).
>
> Done.
>
>
https://codereview.chromium.org/690903002/diff/140001/content/browser/rendere...
> content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:194:
//
> Bitwise-or of SocketOption flags. This stores the state about whether
> On 2014/11/06 01:27:55, bbudge wrote:
> > nit: doesn't match enum name above.
>
> Acknowledged.
>
>
https://codereview.chromium.org/690903002/diff/140001/ppapi/tests/test_udp_so...
> File ppapi/tests/test_udp_socket.cc (right):
>
>
https://codereview.chromium.org/690903002/diff/140001/ppapi/tests/test_udp_so...
> ppapi/tests/test_udp_socket.cc:290: // All ADDRESS_REUSE, BROADCAST,
> SEND_BUFFER_SIZE and RECV_BUFFER_SIZE can
> On 2014/11/06 01:27:55, bbudge wrote:
> > s/All // or
> > s/All /All of/
>
> Done.
jar@, which layer do you worry about? For PPAPI? or for net/... implementation? For PPAPI, we probably tweak somehow, but in either way, I think we need to change net/... somehow similar to this CL. Except backward compatibility, I think this CL simply illustrates what we'd like to do, and I don't have better alternatives right now. If your concern is for net/..., could you share ideas about alternatives? dmichael@, as for generator.py, yes, I tried. What I did is adding a new version, M40 = 1.2 to TCP, and M40 = 1.1 to UDP, without any modification of the interface (actually we do not need to change the interface itself). However, generator.py does NOT create a new interface. It seems an expected behavior, rather than a bug? For example, FileIO (ppb_file_io.idl) has the similar situation. There is M29 = 1.2 label and no new API for FileIO at version 1.2. Then, ppapi/c/ppb_file_io.h contains only 1.0 and 1.1 interfaces. Is it a bug? or am I missing something?
On Thu, Nov 6, 2014 at 12:45 PM, <hidehiko@chromium.org> wrote: > jar@, which layer do you worry about? For PPAPI? or for net/... > implementation? > > For PPAPI, we probably tweak somehow, but in either way, I think we need to > change net/... somehow similar to this CL. > Except backward compatibility, I think this CL simply illustrates what > we'd like > to do, and I don't have better alternatives right now. If your concern is > for > net/..., could you share ideas about alternatives? > > dmichael@, as for generator.py, yes, I tried. > What I did is adding a new version, M40 = 1.2 to TCP, and M40 = 1.1 to UDP, > without any modification of the interface (actually we do not need to > change the > interface itself). To get the IDL generator to generate a new API, you have to copy the affected method(s) and add a version annotation on the new method (1.2 for TCP, 1.1 for UDP). That should force it to generate a new struct and interface string for each of them. If it doesn't, let me know. > However, generator.py does NOT create a new interface. It > seems an expected behavior, rather than a bug? For example, FileIO > (ppb_file_io.idl) has the similar situation. There is M29 = 1.2 label and > no new > API for FileIO at version 1.2. Then, ppapi/c/ppb_file_io.h contains only > 1.0 and > 1.1 interfaces. Is it a bug? or am I missing something? That's a little different, since the version annotation was added to a new enum value and not to any function... the IDL generator is definitely limited when it comes to recognizing that kind of change. It won't applying a version rev to any API that has a function that takes that struct as a parm. We probably *should* have added a new version of PPB_FileIO::Open so that plugins could detect whether it was legal to append, but I guess that didn't happen in that case. > > https://codereview.chromium.org/690903002/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #10 (id:220001) has been deleted
hidehiko@chromium.org changed reviewers: + jar@chromium.org
Thank you for advice, Dave. Versionized. PTAL. Added jar@ to R= again, for histograms.xml. Best regards, - hidehiko
jar@chromium.org changed reviewers: + asvitkine@chromium.org
jar@chromium.org changed required reviewers: + rvargas@chromium.org
Added asvitkine for (tiny/good looking) histograms.xml review, so as to avoid confusion with overall net/* review.
Thanks for versioning the API. This is almost there, just a few comments. https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:195: // each option is set. To make the comment a little clearer, how about this? // Bitwise-or of SocketOption flags. This stores the state about whether // each option is set before Connect() is called. https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:113: // Bitwise-or of SocketOption, which is set before Bind(). To make the comment a little clearer, how about this? // Bitwise-or of SocketOption flags. This stores the state about whether // each option is set before Bind() is called. https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource.h (right): https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource.h:47: nit: extra whitespace https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: if (check_bind_state && bind_called_) Why not use bound_ here? If there is a reason for bind_called_, I think it needs a comment somewhere.
histograms.xml lgtm
Thank you for review. PTAL. https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:195: // each option is set. On 2014/11/07 17:50:39, bbudge wrote: > To make the comment a little clearer, how about this? > > // Bitwise-or of SocketOption flags. This stores the state about whether > // each option is set before Connect() is called. Done. https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:113: // Bitwise-or of SocketOption, which is set before Bind(). On 2014/11/07 17:50:39, bbudge wrote: > To make the comment a little clearer, how about this? > > // Bitwise-or of SocketOption flags. This stores the state about whether > // each option is set before Bind() is called. Done. https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource.h (right): https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource.h:47: On 2014/11/07 17:50:39, bbudge wrote: > nit: extra whitespace Done. https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/240001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: if (check_bind_state && bind_called_) On 2014/11/07 17:50:39, bbudge wrote: > Why not use bound_ here? If there is a reason for bind_called_, I think it needs > a comment somewhere. Using |bound_| here could cause a minor timing issue that these options can be set while in-flight Bind() invocation. Added comments.
LGTM thanks for doing this.
Whoops, 1 comment, still LGTM https://codereview.chromium.org/690903002/diff/260001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/690903002/diff/260001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:113: // the timing on which these are set are slightly different. Could we reword this? 111 // |bind_called_| is true if Bind() is called, while |bound_| is true 112 // after Bind() succeeds. Bind() is an asynchronous method, so 113 // the timing on which of these is set is slightly different.
Thank you for review, Bill, Alexei. Ricardo, Scott, could you review as net OWNER? M40 cutoff is approaching, so we'd like to have your comments. Thanks, - hidehiko https://codereview.chromium.org/690903002/diff/260001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/690903002/diff/260001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:113: // the timing on which these are set are slightly different. On 2014/11/07 21:39:14, bbudge wrote: > Could we reword this? > > 111 // |bind_called_| is true if Bind() is called, while |bound_| is true > 112 // after Bind() succeeds. Bind() is an asynchronous method, so > 113 // the timing on which of these is set is slightly different. Done.
I made a first pass (very superficial). Regarding the branch, a change like this should never land in the last week before a branch. https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:488: int result = SetReceiveBufferSizeInternal(size); I actually don't see why we want to embrace this model of caching everything only to call setsockopt multiple times in a row later on. Is it to protect against caller calling multiple times this method in a row before bind? It looks more twisted and there are other ways to prevent that (*if* we really care) https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:512: int result = SetAddressReuseInternal(true); This path should fail so there is no reason to continue. "Note When using bind with the SO_EXCLUSIVEADDRUSE or SO_REUSEADDR socket option, the socket option must be set prior to executing bind to have any affect."[sic] http://msdn.microsoft.com/en-us/library/windows/desktop/ms737550(v=vs.85).aspx It probably doesn't make sense to change our API unless there's a strong reason to allow clients to attempt this later only to receive a failure. https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:531: int UDPSocketWin::AllowBroadcast() { I'd prefer not having two independent methods to control the same thing, especially considering the behavior of SetMulticastLoopbackMode https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:818: int opt_value = broadcast ? 1 : 0; BOOL, TRUE, FALSE
Thank you for review and comments. So, I looked into SO_REUSEADDR a bit more carefully, and it turned out that its detailed behavior is different between linux and Windows at least. The case is 4) in my previous comment, which is: bind(s1, some-address) setsockopt(s1, SO_REUSEADDR) setsockopt(s2, SO_REUSEADDR) bind(s2, same-address-with-s1) On linux, the second bind() succeeds, while it fails on Windows. Note that setsockopt for s1 (bound socket) succeeds even on Windows. I thought about alternative implementation, but it seems like there is no good way to implement linux's behavior on Windows. So, probably our options for PPAPI change would be: 1) Let platform decide the behavior. 2) Align to Windows' behavior. (i.e. return error on second bind() even on Linux platform). This can be easily doable by ignoring setsockopt(SO_REUSEADDR) for bound socket. 3) Return setsockopt() for bound socket. (This is current behavior). 4) Anything else? IMHO, 2) would be a better approach. WDYT from PPAPI design point of view, Bill? If we choose 2) or 3), we do not need to change net/'s udp socket for REUSEADDR. Also, regardless of REUSEADDR, I would still like to remove timing limitations for other options (TCP_NODELAY for tcp socket, BROADCAST for udp socket, RCVBUF_SIZE, SNDBUF_SIZE for both), and so I'd like to add these changes to net/'s udp socket. Ricardo, do you have any concerns on them, too? Thanks, - hidehiko On 2014/11/08 02:17:20, rvargas wrote: > I made a first pass (very superficial). > > Regarding the branch, a change like this should never land in the last week > before a branch. > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.cc > File net/udp/udp_socket_win.cc (right): > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > net/udp/udp_socket_win.cc:488: int result = SetReceiveBufferSizeInternal(size); > I actually don't see why we want to embrace this model of caching everything > only to call setsockopt multiple times in a row later on. Is it to protect > against caller calling multiple times this method in a row before bind? It looks > more twisted and there are other ways to prevent that (*if* we really care) > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > net/udp/udp_socket_win.cc:512: int result = SetAddressReuseInternal(true); > This path should fail so there is no reason to continue. > > "Note When using bind with the SO_EXCLUSIVEADDRUSE or SO_REUSEADDR socket > option, the socket option must be set prior to executing bind to have any > affect."[sic] > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms737550(v=vs.85).aspx > > It probably doesn't make sense to change our API unless there's a strong reason > to allow clients to attempt this later only to receive a failure. > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > net/udp/udp_socket_win.cc:531: int UDPSocketWin::AllowBroadcast() { > I'd prefer not having two independent methods to control the same thing, > especially considering the behavior of SetMulticastLoopbackMode > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > net/udp/udp_socket_win.cc:818: int opt_value = broadcast ? 1 : 0; > BOOL, TRUE, FALSE
On 2014/11/11 16:48:49, hidehiko wrote: > Thank you for review and comments. > > So, I looked into SO_REUSEADDR a bit more carefully, and it turned out that > its detailed behavior is different between linux and Windows at least. > > The case is 4) in my previous comment, which is: > bind(s1, some-address) > setsockopt(s1, SO_REUSEADDR) > setsockopt(s2, SO_REUSEADDR) > bind(s2, same-address-with-s1) > > On linux, the second bind() succeeds, while it fails on Windows. > Note that setsockopt for s1 (bound socket) succeeds even on Windows. > I thought about alternative implementation, but it seems like there is no good > way to implement linux's > behavior on Windows. So, probably our options for PPAPI change would be: > 1) Let platform decide the behavior. > 2) Align to Windows' behavior. (i.e. return error on second bind() even on Linux > platform). > This can be easily doable by ignoring setsockopt(SO_REUSEADDR) for bound > socket. > 3) Return setsockopt() for bound socket. (This is current behavior). > 4) Anything else? > > IMHO, 2) would be a better approach. WDYT from PPAPI design point of view, Bill? Pepper is intended to be platform neutral, so any behavior that depends on the platform is undesirable. It makes plugin writing even more difficult. So 2) sounds best to me. > If we choose 2) or 3), we do not need to change net/'s udp socket for REUSEADDR. > > Also, regardless of REUSEADDR, I would still like to remove timing limitations > for other options > (TCP_NODELAY for tcp socket, BROADCAST for udp socket, RCVBUF_SIZE, SNDBUF_SIZE > for both), > and so I'd like to add these changes to net/'s udp socket. > Ricardo, do you have any concerns on them, too? > > Thanks, > - hidehiko > > > On 2014/11/08 02:17:20, rvargas wrote: > > I made a first pass (very superficial). > > > > Regarding the branch, a change like this should never land in the last week > > before a branch. > > > > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.cc > > File net/udp/udp_socket_win.cc (right): > > > > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > > net/udp/udp_socket_win.cc:488: int result = > SetReceiveBufferSizeInternal(size); > > I actually don't see why we want to embrace this model of caching everything > > only to call setsockopt multiple times in a row later on. Is it to protect > > against caller calling multiple times this method in a row before bind? It > looks > > more twisted and there are other ways to prevent that (*if* we really care) > > > > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > > net/udp/udp_socket_win.cc:512: int result = SetAddressReuseInternal(true); > > This path should fail so there is no reason to continue. > > > > "Note When using bind with the SO_EXCLUSIVEADDRUSE or SO_REUSEADDR socket > > option, the socket option must be set prior to executing bind to have any > > affect."[sic] > > > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms737550(v=vs.85).aspx > > > > It probably doesn't make sense to change our API unless there's a strong > reason > > to allow clients to attempt this later only to receive a failure. > > > > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > > net/udp/udp_socket_win.cc:531: int UDPSocketWin::AllowBroadcast() { > > I'd prefer not having two independent methods to control the same thing, > > especially considering the behavior of SetMulticastLoopbackMode > > > > > https://codereview.chromium.org/690903002/diff/280001/net/udp/udp_socket_win.... > > net/udp/udp_socket_win.cc:818: int opt_value = broadcast ? 1 : 0; > > BOOL, TRUE, FALSE Comments inline above. Also, this CL should be of interest to you if you haven't seen it already: https://codereview.chromium.org/704133005/
On 2014/11/11 16:48:49, hidehiko wrote: > Thank you for review and comments. > > So, I looked into SO_REUSEADDR a bit more carefully, and it turned out that > its detailed behavior is different between linux and Windows at least. > > The case is 4) in my previous comment, which is: > bind(s1, some-address) > setsockopt(s1, SO_REUSEADDR) > setsockopt(s2, SO_REUSEADDR) > bind(s2, same-address-with-s1) > > On linux, the second bind() succeeds, while it fails on Windows. > Note that setsockopt for s1 (bound socket) succeeds even on Windows. > I thought about alternative implementation, but it seems like there is no good > way to implement linux's > behavior on Windows. So, probably our options for PPAPI change would be: > 1) Let platform decide the behavior. > 2) Align to Windows' behavior. (i.e. return error on second bind() even on Linux > platform). > This can be easily doable by ignoring setsockopt(SO_REUSEADDR) for bound > socket. > 3) Return setsockopt() for bound socket. (This is current behavior). > 4) Anything else? > > IMHO, 2) would be a better approach. WDYT from PPAPI design point of view, Bill? > If we choose 2) or 3), we do not need to change net/'s udp socket for REUSEADDR. > > Also, regardless of REUSEADDR, I would still like to remove timing limitations > for other options > (TCP_NODELAY for tcp socket, BROADCAST for udp socket, RCVBUF_SIZE, SNDBUF_SIZE > for both), > and so I'd like to add these changes to net/'s udp socket. > Ricardo, do you have any concerns on them, too? > > Thanks, > - hidehiko From the point of view of net: Taking a step back, I don't think I understand why we support SO_REUSEADDR at all (and I lean more towards removing that from our code). TCP_NODELAY, RCVBUF_SIZE, SNDBUF_SIZE: Sounds fine to me to allow clients to optimize this on-flight if they think they can :) (current behavior) BROADCAST: I don't have a strong opinion about whether this should be allowed after bind or not. It seems slightly cleaner to me to allow it only before bind and make it a contract to close the socket and create another one if a different mode of operation is desired. I think the model is easier to follow that way... but I may be missing something that makes it performance critical to avoid the overhead of getting a new socket. The second point to consider is that the socket layer is not only there for external consumers but also as a basic layer for the rest of code within net so it should integrate well with the other layers (and their future direction). That's important in the broadcast case, although I don't think we currently use that option. It is better to have APIs that prevent improper use. I have not been following all the work that exposes lower layer to consumers, but I believe Sleevi has, so I would like him to weight on the "change operation mode at any time" vs "create another socket"
+ Ryan (^^)
Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY, and buf sizes. As for BROADCAST, setting it bind() is actually what the main case we want to fix. I think about recreating the socket, but it would introduce timing issue. s1 = socket(...); bind(s1, ...); setsockopt(s1, BROADCAST); Let's assume s1 is not SO_REUSEADDR. If we create another socket and re-bind() it to the same address, then we need to close() s1, before the rebinding. It means, there is a chance for other process/thread to interrupt it (i.e., bind() the same address while it is closed). Without allowing set BROADCAST after bind(), I do not think there is a way to avoid such an interruption. That's why I would like to make a change in net/..., as it looks only place where we can fix the problem. Thanks, - hidehiko On 2014/11/12 01:34:29, rvargas wrote: > + Ryan (^^)
Hmm, Ryan seems OOO for a while. Ricardo, is there anyone else we can ask? Thanks, - hidehiko On 2014/11/12 02:20:02, hidehiko wrote: > Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY, and buf > sizes. > > As for BROADCAST, setting it bind() is actually what the main case we want to > fix. > I think about recreating the socket, but it would introduce timing issue. > > s1 = socket(...); > bind(s1, ...); > setsockopt(s1, BROADCAST); > > Let's assume s1 is not SO_REUSEADDR. > If we create another socket and re-bind() it to the same address, > then we need to close() s1, before the rebinding. > It means, there is a chance for other process/thread to interrupt it > (i.e., bind() the same address while it is closed). > > Without allowing set BROADCAST after bind(), I do not think there is a way to > avoid such an interruption. > That's why I would like to make a change in net/..., as it looks only place > where we can fix the problem. > > Thanks, > - hidehiko > > On 2014/11/12 01:34:29, rvargas wrote: > > + Ryan (^^)
On 2014/11/12 02:43:10, hidehiko wrote: > Hmm, Ryan seems OOO for a while. > Ricardo, is there anyone else we can ask? I think he's been reading email so far... let's wait to see if he sees the message or not. > > Thanks, > - hidehiko > > On 2014/11/12 02:20:02, hidehiko wrote: > > Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY, and buf > > sizes. > > > > As for BROADCAST, setting it bind() is actually what the main case we want to > > fix. > > I think about recreating the socket, but it would introduce timing issue. > > > > s1 = socket(...); > > bind(s1, ...); > > setsockopt(s1, BROADCAST); > > > > Let's assume s1 is not SO_REUSEADDR. > > If we create another socket and re-bind() it to the same address, > > then we need to close() s1, before the rebinding. > > It means, there is a chance for other process/thread to interrupt it > > (i.e., bind() the same address while it is closed). > > > > Without allowing set BROADCAST after bind(), I do not think there is a way to > > avoid such an interruption. > > That's why I would like to make a change in net/..., as it looks only place > > where we can fix the problem. Well, yes, but why is that a problem? Isn't there something wrong to start with if the app has to be "fighting" with someone else for that specific port? If that is really an issue where's the guarantee that makes it so that s1 can bind in the first place? > > > > Thanks, > > - hidehiko > > > > On 2014/11/12 01:34:29, rvargas wrote: > > > + Ryan (^^)
On 2014/11/12 03:21:21, rvargas wrote: > On 2014/11/12 02:43:10, hidehiko wrote: > > Hmm, Ryan seems OOO for a while. > > Ricardo, is there anyone else we can ask? > > I think he's been reading email so far... let's wait to see if he sees the > message or not. Ok. Thank you for advice. > > > > > Thanks, > > - hidehiko > > > > On 2014/11/12 02:20:02, hidehiko wrote: > > > Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY, and buf > > > sizes. > > > > > > As for BROADCAST, setting it bind() is actually what the main case we want > to > > > fix. > > > I think about recreating the socket, but it would introduce timing issue. > > > > > > s1 = socket(...); > > > bind(s1, ...); > > > setsockopt(s1, BROADCAST); > > > > > > Let's assume s1 is not SO_REUSEADDR. > > > If we create another socket and re-bind() it to the same address, > > > then we need to close() s1, before the rebinding. > > > It means, there is a chance for other process/thread to interrupt it > > > (i.e., bind() the same address while it is closed). > > > > > > Without allowing set BROADCAST after bind(), I do not think there is a way > to > > > avoid such an interruption. > > > That's why I would like to make a change in net/..., as it looks only place > > > where we can fix the problem. > > Well, yes, but why is that a problem? Isn't there something wrong to start with > if the app has to be "fighting" with someone else for that specific port? If > that is really an issue where's the guarantee that makes it so that s1 can bind > in the first place? Practically, it shouldn't have such a situation, but we cannot control developers' usage, in general. Also, from API design point of view, I don't think it is healthy to keep such a pit hole. From users point of view, if an user hits such a issue, it'd be hard to understand what's going on, and probably it would cause flaky bugs which are difficult to be reproduced. Admittedly, we can work around to introduce BROADCAST after bind() except the buggy situation, but at the same time, we can simply avoid the situation by allowing BROADCAST after bind(), too. So, the allowing looks simpler, consistent and less confusing solution to me? > > > > > > > > Thanks, > > > - hidehiko > > > > > > On 2014/11/12 01:34:29, rvargas wrote: > > > > + Ryan (^^)
On Tue, Nov 11, 2014 at 6:20 PM, <hidehiko@chromium.org> wrote: > > Let's assume s1 is not SO_REUSEADDR. > If we create another socket and re-bind() it to the same address, > then we need to close() s1, before the rebinding. > It means, there is a chance for other process/thread to interrupt it > (i.e., bind() the same address while it is closed). > Random peanut-gallery comment: I think the point of SO_REUSEADDR is so that the receive socket can be re-opened without having to wait 2 minutes for TIME_WAIT to clear on any closed connections to that endpoint. I might be thinking of a different TCP thing, though. https://codereview.chromium.org/690903002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/11/12 04:01:24, hidehiko wrote: > On 2014/11/12 03:21:21, rvargas wrote: > > On 2014/11/12 02:43:10, hidehiko wrote: > > > Hmm, Ryan seems OOO for a while. > > > Ricardo, is there anyone else we can ask? > > > > I think he's been reading email so far... let's wait to see if he sees the > > message or not. > > Ok. Thank you for advice. > > > > > > > > > Thanks, > > > - hidehiko > > > > > > On 2014/11/12 02:20:02, hidehiko wrote: > > > > Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY, and > buf > > > > sizes. > > > > > > > > As for BROADCAST, setting it bind() is actually what the main case we want > > to > > > > fix. > > > > I think about recreating the socket, but it would introduce timing issue. > > > > > > > > s1 = socket(...); > > > > bind(s1, ...); > > > > setsockopt(s1, BROADCAST); > > > > > > > > Let's assume s1 is not SO_REUSEADDR. > > > > If we create another socket and re-bind() it to the same address, > > > > then we need to close() s1, before the rebinding. > > > > It means, there is a chance for other process/thread to interrupt it > > > > (i.e., bind() the same address while it is closed). > > > > > > > > Without allowing set BROADCAST after bind(), I do not think there is a way > > to > > > > avoid such an interruption. > > > > That's why I would like to make a change in net/..., as it looks only > place > > > > where we can fix the problem. > > > > Well, yes, but why is that a problem? Isn't there something wrong to start > with > > if the app has to be "fighting" with someone else for that specific port? If > > that is really an issue where's the guarantee that makes it so that s1 can > bind > > in the first place? > > Practically, it shouldn't have such a situation, but we cannot control > developers' usage, in general. An API guides what someone can do with it. > Also, from API design point of view, I don't think it is healthy to keep such > a pit hole. From users point of view, if an user hits such a issue, it'd be > hard to understand what's going on, and probably it would cause flaky bugs > which are difficult to be reproduced. I'm sorry, but I still don't see what would be issue / flaky behavior that the dev would face that would not be present in the first bind attempt. > Admittedly, we can work around to introduce BROADCAST after bind() except the > buggy situation, > but at the same time, we can simply avoid the situation by allowing BROADCAST > after bind(), too. > So, the allowing looks simpler, consistent and less confusing solution to me? > Are you are calling a bug that someone else can grab the port between close and the second bind? Or are you referring to something else?. If that is what you mean, how is that different from someone else grabbing the port before the first bind? How come the second failure would be unexpected or flaky but not the first failure? At the end it's not a big deal; as I said I really don't have a strong opinion here, but I want to make sure I understand the motivation. I'm not sure we want to be a thin wrapper around every OS provided API. > > > > > > > > > > > > Thanks, > > > > - hidehiko > > > > > > > > On 2014/11/12 01:34:29, rvargas wrote: > > > > > + Ryan (^^)
On 2014/11/12 06:02:35, Scott Hess - On Leave wrote: > On Tue, Nov 11, 2014 at 6:20 PM, <mailto:hidehiko@chromium.org> wrote: > > > > Let's assume s1 is not SO_REUSEADDR. > > If we create another socket and re-bind() it to the same address, > > then we need to close() s1, before the rebinding. > > It means, there is a chance for other process/thread to interrupt it > > (i.e., bind() the same address while it is closed). > > > > Random peanut-gallery comment: I think the point of SO_REUSEADDR is so that > the receive socket can be re-opened without having to wait 2 minutes for > TIME_WAIT to clear on any closed connections to that endpoint. I might be > thinking of a different TCP thing, though. I was thinking along those lines too, but that still sounds quite dangerous to me. > > https://codereview.chromium.org/690903002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/11/12 19:01:58, rvargas wrote:
> On 2014/11/12 04:01:24, hidehiko wrote:
> > On 2014/11/12 03:21:21, rvargas wrote:
> > > On 2014/11/12 02:43:10, hidehiko wrote:
> > > > Hmm, Ryan seems OOO for a while.
> > > > Ricardo, is there anyone else we can ask?
> > >
> > > I think he's been reading email so far... let's wait to see if he sees the
> > > message or not.
> >
> > Ok. Thank you for advice.
> >
> > >
> > > >
> > > > Thanks,
> > > > - hidehiko
> > > >
> > > > On 2014/11/12 02:20:02, hidehiko wrote:
> > > > > Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY,
and
> > buf
> > > > > sizes.
> > > > >
> > > > > As for BROADCAST, setting it bind() is actually what the main case we
> want
> > > to
> > > > > fix.
> > > > > I think about recreating the socket, but it would introduce timing
> issue.
> > > > >
> > > > > s1 = socket(...);
> > > > > bind(s1, ...);
> > > > > setsockopt(s1, BROADCAST);
> > > > >
> > > > > Let's assume s1 is not SO_REUSEADDR.
> > > > > If we create another socket and re-bind() it to the same address,
> > > > > then we need to close() s1, before the rebinding.
> > > > > It means, there is a chance for other process/thread to interrupt it
> > > > > (i.e., bind() the same address while it is closed).
> > > > >
> > > > > Without allowing set BROADCAST after bind(), I do not think there is a
> way
> > > to
> > > > > avoid such an interruption.
> > > > > That's why I would like to make a change in net/..., as it looks only
> > place
> > > > > where we can fix the problem.
> > >
> > > Well, yes, but why is that a problem? Isn't there something wrong to start
> > with
> > > if the app has to be "fighting" with someone else for that specific port?
If
> > > that is really an issue where's the guarantee that makes it so that s1 can
> > bind
> > > in the first place?
> >
> > Practically, it shouldn't have such a situation, but we cannot control
> > developers' usage, in general.
>
> An API guides what someone can do with it.
>
I agree that API should guides, admittedly.
However, the situation is not that easy, unfortunately.
We need setsockopt compatible implementation.
PPAPI's {TCP,UDP}Socket::SetOption is the ones we're using, but it has some
inconsistency (timing limitation).
Actually some App hit the real issue (crbug.com/425563) by
setsockopt(SO_BROADCAST) after bind(), which should work.
So, we need to fix the behavior. This is the starting point.
It means; if BROADCAST should not be set after bind(),
navigation by API should be returning an error for that (as the current PPAPI
does).
However, it does not satisfy our requirements, because implementing the
setsockopt behavior
on top of it leaves timing issue, i.e. it cannot be implemented correctly, as I
wrote.
> > Also, from API design point of view, I don't think it is healthy to keep
such
> > a pit hole. From users point of view, if an user hits such a issue, it'd be
> > hard to understand what's going on, and probably it would cause flaky bugs
> > which are difficult to be reproduced.
>
> I'm sorry, but I still don't see what would be issue / flaky behavior that the
> dev would face that would not be present in the first bind attempt.
>
>
> > Admittedly, we can work around to introduce BROADCAST after bind() except
the
> > buggy situation,
> > but at the same time, we can simply avoid the situation by allowing
BROADCAST
> > after bind(), too.
> > So, the allowing looks simpler, consistent and less confusing solution to
me?
> >
>
> Are you are calling a bug that someone else can grab the port between close
and
> the second bind? Or are you referring to something else?. If that is what you
> mean, how is that different from someone else grabbing the port before the
first
> bind? How come the second failure would be unexpected or flaky but not the
first
> failure?
>
From my point of view, the former case is "the port is reserved/used by other
process,"
but the latter case is "the port is suddenly stolen during setsockopt()
invocation."
How client app works is up to the app itself, but "reserved port" is more common
case than "stolen port"
for most of programmers, I think. So, app could be flakier with "stolen port"
case,
because programmers wouldn't assume such a case.
Also, re-bind() is the "implementation details" from the users and should be
hidden/encapsulated in general.
Thus, it would be very hard to understand what's going on then.
Stepping back, as you said, this should be very rare case, or should unlikely
happen (I hope).
However, I'd like to have implementation as correct as possible.
We already hit some nightmare bugs, which are caused by some tiny
inconsistent/flaky behavior
between PPAPIs and posix.
I know, I may be too nervous, but such a bug is what we fight against...
In general, any bug could bite ourselves even if it looks very rare.
So, I would like to avoid such a situation if possible.
> At the end it's not a big deal; as I said I really don't have a strong opinion
> here, but I want to make sure I understand the motivation. I'm not sure we
want
> to be a thin wrapper around every OS provided API.
Our main motivation is, implementing setsockopt behavior, including to allow
SO_BROADCAST after bind() properly
(i.e. without leaving bugs (incl. timing issues) even if these should be very
rare).
If there is an alternative way, I'm fine to leave AllowBroadcast() as is, but I
have no idea about it.
(Note that Re-bind() does not work, because it leaves timing issue).
At least for now, changing it to SetBroadcast(bool) looks better approach.
From my point of view, its semantics look clear and straightforward enough (even
compared to AllowBroadcast()),
because it can be called whenever and effective. Also, the implementation is
straightforward, too.
Surely, we are not necessary to be a thin wrapper of OS APIs,
but we really need enough APIs for setsockopt compatible implementation.
Currently, PPAPI SetOption is not enough, unfortunately, so I'm trying to
improve it.
BTW, because this CL became bigger, I extract net/... related part into another
CL.
https://codereview.chromium.org/721273002/
Thanks,
- hidehiko
>
> > >
> > >
> > > > >
> > > > > Thanks,
> > > > > - hidehiko
> > > > >
> > > > > On 2014/11/12 01:34:29, rvargas wrote:
> > > > > > + Ryan (^^)
On 2014/11/13 17:55:18, hidehiko wrote:
> On 2014/11/12 19:01:58, rvargas wrote:
> > On 2014/11/12 04:01:24, hidehiko wrote:
> > > On 2014/11/12 03:21:21, rvargas wrote:
> > > > On 2014/11/12 02:43:10, hidehiko wrote:
> > > > > Hmm, Ryan seems OOO for a while.
> > > > > Ricardo, is there anyone else we can ask?
> > > >
> > > > I think he's been reading email so far... let's wait to see if he sees
the
> > > > message or not.
> > >
> > > Ok. Thank you for advice.
> > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > - hidehiko
> > > > >
> > > > > On 2014/11/12 02:20:02, hidehiko wrote:
> > > > > > Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY,
> and
> > > buf
> > > > > > sizes.
> > > > > >
> > > > > > As for BROADCAST, setting it bind() is actually what the main case
we
> > want
> > > > to
> > > > > > fix.
> > > > > > I think about recreating the socket, but it would introduce timing
> > issue.
> > > > > >
> > > > > > s1 = socket(...);
> > > > > > bind(s1, ...);
> > > > > > setsockopt(s1, BROADCAST);
> > > > > >
> > > > > > Let's assume s1 is not SO_REUSEADDR.
> > > > > > If we create another socket and re-bind() it to the same address,
> > > > > > then we need to close() s1, before the rebinding.
> > > > > > It means, there is a chance for other process/thread to interrupt it
> > > > > > (i.e., bind() the same address while it is closed).
> > > > > >
> > > > > > Without allowing set BROADCAST after bind(), I do not think there is
a
> > way
> > > > to
> > > > > > avoid such an interruption.
> > > > > > That's why I would like to make a change in net/..., as it looks
only
> > > place
> > > > > > where we can fix the problem.
> > > >
> > > > Well, yes, but why is that a problem? Isn't there something wrong to
start
> > > with
> > > > if the app has to be "fighting" with someone else for that specific
port?
> If
> > > > that is really an issue where's the guarantee that makes it so that s1
can
> > > bind
> > > > in the first place?
> > >
> > > Practically, it shouldn't have such a situation, but we cannot control
> > > developers' usage, in general.
> >
> > An API guides what someone can do with it.
> >
>
>
> I agree that API should guides, admittedly.
> However, the situation is not that easy, unfortunately.
> We need setsockopt compatible implementation.
> PPAPI's {TCP,UDP}Socket::SetOption is the ones we're using, but it has some
> inconsistency (timing limitation).
> Actually some App hit the real issue (crbug.com/425563) by
> setsockopt(SO_BROADCAST) after bind(), which should work.
> So, we need to fix the behavior. This is the starting point.
>
> It means; if BROADCAST should not be set after bind(),
> navigation by API should be returning an error for that (as the current PPAPI
> does).
> However, it does not satisfy our requirements, because implementing the
> setsockopt behavior
> on top of it leaves timing issue, i.e. it cannot be implemented correctly, as
I
> wrote.
>
> > > Also, from API design point of view, I don't think it is healthy to keep
> such
> > > a pit hole. From users point of view, if an user hits such a issue, it'd
be
> > > hard to understand what's going on, and probably it would cause flaky bugs
> > > which are difficult to be reproduced.
> >
> > I'm sorry, but I still don't see what would be issue / flaky behavior that
the
> > dev would face that would not be present in the first bind attempt.
> >
> >
> > > Admittedly, we can work around to introduce BROADCAST after bind() except
> the
> > > buggy situation,
> > > but at the same time, we can simply avoid the situation by allowing
> BROADCAST
> > > after bind(), too.
> > > So, the allowing looks simpler, consistent and less confusing solution to
> me?
> > >
> >
> > Are you are calling a bug that someone else can grab the port between close
> and
> > the second bind? Or are you referring to something else?. If that is what
you
> > mean, how is that different from someone else grabbing the port before the
> first
> > bind? How come the second failure would be unexpected or flaky but not the
> first
> > failure?
> >
>
> From my point of view, the former case is "the port is reserved/used by other
> process,"
> but the latter case is "the port is suddenly stolen during setsockopt()
> invocation."
> How client app works is up to the app itself, but "reserved port" is more
common
> case than "stolen port"
> for most of programmers, I think.
That's an interesting way to put it. But you have not addressed why "reserved"
is not strictly a subset of "stolen". Or to say it in another way, anybody who
is subject to the "stolen" use case needs to worry about the thief stealing the
port before the first bind (making the distinction between the two cases
irrelevant).
- If there is no need to worry about stealing then there is no need to change
the mode of operation in-flight.
- If there is a need to worry about stealing then there is no way to securely
grab the port in the first place (or the same mechanism that copes with it can
be used when the port is gone).
> So, app could be flakier with "stolen port"
> case,
> because programmers wouldn't assume such a case.
> Also, re-bind() is the "implementation details" from the users and should be
> hidden/encapsulated in general.
> Thus, it would be very hard to understand what's going on then.
>
> Stepping back, as you said, this should be very rare case, or should unlikely
> happen (I hope).
> However, I'd like to have implementation as correct as possible.
> We already hit some nightmare bugs, which are caused by some tiny
> inconsistent/flaky behavior
> between PPAPIs and posix.
> I know, I may be too nervous, but such a bug is what we fight against...
> In general, any bug could bite ourselves even if it looks very rare.
> So, I would like to avoid such a situation if possible.
>
> > At the end it's not a big deal; as I said I really don't have a strong
opinion
> > here, but I want to make sure I understand the motivation. I'm not sure we
> want
> > to be a thin wrapper around every OS provided API.
I'm glad you say that, but the only motivation that I see at this point is "we
want to promise POSIX semantics to our users" and that basically translates in
net exposing a thin wrapper around the OS API :(
BTW, I filed bug 433024 to remove AllowAddressReuse()
>
>
> Our main motivation is, implementing setsockopt behavior, including to allow
> SO_BROADCAST after bind() properly
> (i.e. without leaving bugs (incl. timing issues) even if these should be very
> rare).
> If there is an alternative way, I'm fine to leave AllowBroadcast() as is, but
I
> have no idea about it.
> (Note that Re-bind() does not work, because it leaves timing issue).
> At least for now, changing it to SetBroadcast(bool) looks better approach.
> From my point of view, its semantics look clear and straightforward enough
(even
> compared to AllowBroadcast()),
> because it can be called whenever and effective. Also, the implementation is
> straightforward, too.
>
> Surely, we are not necessary to be a thin wrapper of OS APIs,
> but we really need enough APIs for setsockopt compatible implementation.
> Currently, PPAPI SetOption is not enough, unfortunately, so I'm trying to
> improve it.
>
> BTW, because this CL became bigger, I extract net/... related part into
another
> CL.
> https://codereview.chromium.org/721273002/
>
> Thanks,
> - hidehiko
>
> >
> > > >
> > > >
> > > > > >
> > > > > > Thanks,
> > > > > > - hidehiko
> > > > > >
> > > > > > On 2014/11/12 01:34:29, rvargas wrote:
> > > > > > > + Ryan (^^)
On 2014/11/13 20:53:21, rvargas wrote:
> On 2014/11/13 17:55:18, hidehiko wrote:
> > On 2014/11/12 19:01:58, rvargas wrote:
> > > On 2014/11/12 04:01:24, hidehiko wrote:
> > > > On 2014/11/12 03:21:21, rvargas wrote:
> > > > > On 2014/11/12 02:43:10, hidehiko wrote:
> > > > > > Hmm, Ryan seems OOO for a while.
> > > > > > Ricardo, is there anyone else we can ask?
> > > > >
> > > > > I think he's been reading email so far... let's wait to see if he sees
> the
> > > > > message or not.
> > > >
> > > > Ok. Thank you for advice.
> > > >
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > - hidehiko
> > > > > >
> > > > > > On 2014/11/12 02:20:02, hidehiko wrote:
> > > > > > > Thank you. So, we are on the same page about REUSEADDR,
TCP_NODELAY,
> > and
> > > > buf
> > > > > > > sizes.
> > > > > > >
> > > > > > > As for BROADCAST, setting it bind() is actually what the main case
> we
> > > want
> > > > > to
> > > > > > > fix.
> > > > > > > I think about recreating the socket, but it would introduce timing
> > > issue.
> > > > > > >
> > > > > > > s1 = socket(...);
> > > > > > > bind(s1, ...);
> > > > > > > setsockopt(s1, BROADCAST);
> > > > > > >
> > > > > > > Let's assume s1 is not SO_REUSEADDR.
> > > > > > > If we create another socket and re-bind() it to the same address,
> > > > > > > then we need to close() s1, before the rebinding.
> > > > > > > It means, there is a chance for other process/thread to interrupt
it
> > > > > > > (i.e., bind() the same address while it is closed).
> > > > > > >
> > > > > > > Without allowing set BROADCAST after bind(), I do not think there
is
> a
> > > way
> > > > > to
> > > > > > > avoid such an interruption.
> > > > > > > That's why I would like to make a change in net/..., as it looks
> only
> > > > place
> > > > > > > where we can fix the problem.
> > > > >
> > > > > Well, yes, but why is that a problem? Isn't there something wrong to
> start
> > > > with
> > > > > if the app has to be "fighting" with someone else for that specific
> port?
> > If
> > > > > that is really an issue where's the guarantee that makes it so that s1
> can
> > > > bind
> > > > > in the first place?
> > > >
> > > > Practically, it shouldn't have such a situation, but we cannot control
> > > > developers' usage, in general.
> > >
> > > An API guides what someone can do with it.
> > >
> >
> >
> > I agree that API should guides, admittedly.
> > However, the situation is not that easy, unfortunately.
> > We need setsockopt compatible implementation.
> > PPAPI's {TCP,UDP}Socket::SetOption is the ones we're using, but it has some
> > inconsistency (timing limitation).
> > Actually some App hit the real issue (crbug.com/425563) by
> > setsockopt(SO_BROADCAST) after bind(), which should work.
> > So, we need to fix the behavior. This is the starting point.
> >
> > It means; if BROADCAST should not be set after bind(),
> > navigation by API should be returning an error for that (as the current
PPAPI
> > does).
> > However, it does not satisfy our requirements, because implementing the
> > setsockopt behavior
> > on top of it leaves timing issue, i.e. it cannot be implemented correctly,
as
> I
> > wrote.
> >
> > > > Also, from API design point of view, I don't think it is healthy to keep
> > such
> > > > a pit hole. From users point of view, if an user hits such a issue, it'd
> be
> > > > hard to understand what's going on, and probably it would cause flaky
bugs
> > > > which are difficult to be reproduced.
> > >
> > > I'm sorry, but I still don't see what would be issue / flaky behavior that
> the
> > > dev would face that would not be present in the first bind attempt.
> > >
> > >
> > > > Admittedly, we can work around to introduce BROADCAST after bind()
except
> > the
> > > > buggy situation,
> > > > but at the same time, we can simply avoid the situation by allowing
> > BROADCAST
> > > > after bind(), too.
> > > > So, the allowing looks simpler, consistent and less confusing solution
to
> > me?
> > > >
> > >
> > > Are you are calling a bug that someone else can grab the port between
close
> > and
> > > the second bind? Or are you referring to something else?. If that is what
> you
> > > mean, how is that different from someone else grabbing the port before the
> > first
> > > bind? How come the second failure would be unexpected or flaky but not the
> > first
> > > failure?
> > >
> >
> > From my point of view, the former case is "the port is reserved/used by
other
> > process,"
> > but the latter case is "the port is suddenly stolen during setsockopt()
> > invocation."
> > How client app works is up to the app itself, but "reserved port" is more
> common
> > case than "stolen port"
> > for most of programmers, I think.
>
> That's an interesting way to put it. But you have not addressed why "reserved"
> is not strictly a subset of "stolen". Or to say it in another way, anybody who
> is subject to the "stolen" use case needs to worry about the thief stealing
the
> port before the first bind (making the distinction between the two cases
> irrelevant).
>
> - If there is no need to worry about stealing then there is no need to change
> the mode of operation in-flight.
> - If there is a need to worry about stealing then there is no way to securely
> grab the port in the first place (or the same mechanism that copes with it can
> be used when the port is gone).
>
Sorry, but I don't understand your meaning.
Why "If there is no need to worry about stealing then there is no need to change
the mode of operation in-flight."?
Changing the socket mode and if port will be stolen or not does not sound
connected each other?
Why "If there is a need to worry about stealing then there is no way to securely
grab the port in the first place (or the same mechanism that copes with it can
be used when the port is gone)."?
There could be some period between the bind() and setsockopt().
Then first bind() succeeds but then stolen?
I'd appreciate more detailed explanation.
> > So, app could be flakier with "stolen port"
> > case,
> > because programmers wouldn't assume such a case.
> > Also, re-bind() is the "implementation details" from the users and should be
> > hidden/encapsulated in general.
> > Thus, it would be very hard to understand what's going on then.
> >
> > Stepping back, as you said, this should be very rare case, or should
unlikely
> > happen (I hope).
> > However, I'd like to have implementation as correct as possible.
> > We already hit some nightmare bugs, which are caused by some tiny
> > inconsistent/flaky behavior
> > between PPAPIs and posix.
> > I know, I may be too nervous, but such a bug is what we fight against...
> > In general, any bug could bite ourselves even if it looks very rare.
> > So, I would like to avoid such a situation if possible.
> >
> > > At the end it's not a big deal; as I said I really don't have a strong
> opinion
> > > here, but I want to make sure I understand the motivation. I'm not sure we
> > want
> > > to be a thin wrapper around every OS provided API.
>
> I'm glad you say that, but the only motivation that I see at this point is "we
> want to promise POSIX semantics to our users" and that basically translates in
> net exposing a thin wrapper around the OS API :(
>
I also do not understand your motivation, then...
POSIX semantics is the requirements for ARC.
The new API incl. semantics looks simple.
We do not need platform-depended hack for its implementation.
What is the main problem here?
- hidehiko
> BTW, I filed bug 433024 to remove AllowAddressReuse()
>
> >
> >
> > Our main motivation is, implementing setsockopt behavior, including to allow
> > SO_BROADCAST after bind() properly
> > (i.e. without leaving bugs (incl. timing issues) even if these should be
very
> > rare).
> > If there is an alternative way, I'm fine to leave AllowBroadcast() as is,
but
> I
> > have no idea about it.
> > (Note that Re-bind() does not work, because it leaves timing issue).
> > At least for now, changing it to SetBroadcast(bool) looks better approach.
> > From my point of view, its semantics look clear and straightforward enough
> (even
> > compared to AllowBroadcast()),
> > because it can be called whenever and effective. Also, the implementation is
> > straightforward, too.
> >
> > Surely, we are not necessary to be a thin wrapper of OS APIs,
> > but we really need enough APIs for setsockopt compatible implementation.
> > Currently, PPAPI SetOption is not enough, unfortunately, so I'm trying to
> > improve it.
> >
> > BTW, because this CL became bigger, I extract net/... related part into
> another
> > CL.
> > https://codereview.chromium.org/721273002/
> >
> > Thanks,
> > - hidehiko
> >
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > - hidehiko
> > > > > > >
> > > > > > > On 2014/11/12 01:34:29, rvargas wrote:
> > > > > > > > + Ryan (^^)
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
I'm sorry, it's still taking time for me to catch up. However, I want to echo Ricardo's concerns, as from what I've read, I'm not really comfortable with the design/approach. Our API design has been explicitly NOT to provide a "general POSIX-compatible API", because that's not an expectation we can meet in a cross-platform way in many areas. We tell this to consumers of //net stack code regularly, and it's come up in past designs (for example, Mojo). For the general health of //net and "Chrome as a Browser", it's better to ensure the API is something that 99% can use correctly and that hides much of the complexity. On more minor points, I'm uncomfortable with the Allow/Disallow pairing of methods, as that is an API smell that leaves ambiguity as to the default state or to the general state. A bool or enum approach is far better, with the latter. For example, SetX(ALLOW_X / DISALLOW_X) and GetX() returning ALLOW_X / DISALLOW_X (or whatever appropriate verb/noun combinations are appropriate for the enum) generally represents a better state. The whole caching + repeated calls also strikes me as an implementation smell that I'm not very keen on either, and it looks like Ricardo has already pointed that out. Given the unease it's given three //net reviewers, I think it may help to more generally discuss this on net-dev@, to ensure there is consensus on what //net/udp should provide to //net-stack consumers, what we're willing and able to support, to understand about your use cases (to the extent that you can share; being unable to share use cases publicly is also generally a red flag), and to see what sort of solutions we can come up given that space. This is fundamentally a question of code health and API design, and I don't want you to take this as a "No, go away", but as a "This is not as trivial as you think, and I also want to make sure we're all on the same page, lest you feel you might need to send this to other reviewers to LG"
On 2014/11/14 01:44:50, hidehiko wrote: I'm cleaning a little this thread to make the response more readable. > > That's an interesting way to put it. But you have not addressed why "reserved" > > is not strictly a subset of "stolen". Or to say it in another way, anybody who > > is subject to the "stolen" use case needs to worry about the thief stealing > the > > port before the first bind (making the distinction between the two cases > > irrelevant). > > > > - If there is no need to worry about stealing then there is no need to change > > the mode of operation in-flight. > > - If there is a need to worry about stealing then there is no way to securely > > grab the port in the first place (or the same mechanism that copes with it can > > be used when the port is gone). > > > > Sorry, but I don't understand your meaning. > Why "If there is no need to worry about stealing then there is no need to change > the mode of operation in-flight."? I'm just making the two cases explicit. What I mean is that if I'm a random app developer and I know that I don't have to worry about losing the port, then no changes to the current API are needed. I believe we agree here, so that means that this is a case that is not very interesting to us for the purpose of this discussion. > Changing the socket mode and if port will be stolen or not does not sound > connected each other? > > Why "If there is a need to worry about stealing then there is no way to securely > grab the port in the first place (or the same mechanism that copes with it can > be used when the port is gone)."? This is the case that is interesting... In this case, I know (as a random app developer), that I have to worry about port squatting (someone else attempting to use the port I want, at any time). In this case, closing the socket and creating a new one presents an opportunity for that squatter to sit in the port that I want. But that squatter can also do that before I grab the port for the first time. My question is why could we say that a squatter can grab the port while I try to change the mode but not before I create the first socket? I don't see any reason to dismiss that scenario to be less likely, because we start from the assumption that there is someone trying to grab that port. And that means that I (as an app dev) have to write my app (protocol or whatever) in a way that I can deal properly with that case: not getting the port that I want when I start, or not getting it when I switch mode. Covering one hole but leaving the other one open doesn't really help me. > There could be some period between the bind() and setsockopt(). > Then first bind() succeeds but then stolen? > > I'd appreciate more detailed explanation. > > > Surely, we are not necessary to be a thin wrapper of OS APIs, > > > but we really need enough APIs for setsockopt compatible implementation. > > I'm glad you say that, but the only motivation that I see at this point is "we > > want to promise POSIX semantics to our users" and that basically translates in > > net exposing a thin wrapper around the OS API :( > > > > I also do not understand your motivation, then... > POSIX semantics is the requirements for ARC. > The new API incl. semantics looks simple. > We do not need platform-depended hack for its implementation. > What is the main problem here? What is ARC? When I read the linked bugs that was mentioned but that is far away from src/net... which means that may motivate changes to PPAPI but not necessarily to net (which is what I'm focusing on here). If we _have_to_ provide POSIX semantics then sure, let's change the mode at any time. If we don't _have_to_ provide POSIX semantics, keeping the same mode for the lifetime of the socket is a little cleaner (IMHO). A pretty convincing counter argument would be performance degradation (we do almost anything for performance), but I don't think that applies in this case. Most likely what's going on is that I have no idea how important is to change the behavior.
On 2014/11/14 02:39:26, rvargas wrote:
> On 2014/11/14 01:44:50, hidehiko wrote:
>
> I'm cleaning a little this thread to make the response more readable.
>
> > > That's an interesting way to put it. But you have not addressed why
> "reserved"
> > > is not strictly a subset of "stolen". Or to say it in another way, anybody
> who
> > > is subject to the "stolen" use case needs to worry about the thief
stealing
> > the
> > > port before the first bind (making the distinction between the two cases
> > > irrelevant).
> > >
> > > - If there is no need to worry about stealing then there is no need to
> change
> > > the mode of operation in-flight.
> > > - If there is a need to worry about stealing then there is no way to
> securely
> > > grab the port in the first place (or the same mechanism that copes with it
> can
> > > be used when the port is gone).
> > >
> >
> > Sorry, but I don't understand your meaning.
> > Why "If there is no need to worry about stealing then there is no need to
> change
> > the mode of operation in-flight."?
>
> I'm just making the two cases explicit.
>
> What I mean is that if I'm a random app developer and I know that I don't have
> to worry about losing the port, then no changes to the current API are needed.
I
> believe we agree here, so that means that this is a case that is not very
> interesting to us for the purpose of this discussion.
>
At which time? On bind() or on setsockopt()? It is what I'm interested in.
If I were a developer, I'd probably imagine an error case on bind() (and write
an error handling),
but I would not think to loose it on setsockopt(). I.e., if setsockopt() failed,
I would imagine it is a missing feature or something, but I would not think the
port was stolen.
I still think "cannot grab the port" and "the grabbed port is stolen" are
different...
Am I missing something?
> > Changing the socket mode and if port will be stolen or not does not sound
> > connected each other?
> >
> > Why "If there is a need to worry about stealing then there is no way to
> securely
> > grab the port in the first place (or the same mechanism that copes with it
can
> > be used when the port is gone)."?
>
> This is the case that is interesting... In this case, I know (as a random app
> developer), that I have to worry about port squatting (someone else attempting
> to use the port I want, at any time). In this case, closing the socket and
> creating a new one presents an opportunity for that squatter to sit in the
port
> that I want. But that squatter can also do that before I grab the port for the
> first time.
>
> My question is why could we say that a squatter can grab the port while I try
to
> change the mode but not before I create the first socket?
> I don't see any reason to dismiss that scenario to be less likely, because we
> start from the assumption that there is someone trying to grab that port. And
> that means that I (as an app dev) have to write my app (protocol or whatever)
in
> a way that I can deal properly with that case: not getting the port that I
want
> when I start, or not getting it when I switch mode.
>
> Covering one hole but leaving the other one open doesn't really help me.
>
> > There could be some period between the bind() and setsockopt().
> > Then first bind() succeeds but then stolen?
> >
> > I'd appreciate more detailed explanation.
>
> > > > Surely, we are not necessary to be a thin wrapper of OS APIs,
> > > > but we really need enough APIs for setsockopt compatible implementation.
> > > I'm glad you say that, but the only motivation that I see at this point is
> "we
> > > want to promise POSIX semantics to our users" and that basically
translates
> in
> > > net exposing a thin wrapper around the OS API :(
> > >
> >
> > I also do not understand your motivation, then...
> > POSIX semantics is the requirements for ARC.
> > The new API incl. semantics looks simple.
> > We do not need platform-depended hack for its implementation.
> > What is the main problem here?
>
> What is ARC? When I read the linked bugs that was mentioned but that is far
away
> from src/net... which means that may motivate changes to PPAPI but not
> necessarily to net (which is what I'm focusing on here).
>
> If we _have_to_ provide POSIX semantics then sure, let's change the mode at
any
> time.
>
Oh, sorry. Please let me share more backgrounds.
I'm working on ARC (App Runtime for Chrome) project, which is to run android
applications on Chrome, launched two months ago.
In the project, we implement posix-layer on top of NaCl IRTs and PPAPIs.
Specifically, we implement socket related posix functions on top of socket
related PPAPIs.
{TCP,UDP}Socket::SetOption is the ones we use for setsockopt implementation.
We are thinking that the compatibility is VERY important. I believe you agree
that,
"in this kind of project", incompatibility easily causes a nightmare bugs, even
if the
incompatibility looks very small. Considering to increase the number of
supported apps,
its risk would be increased rapidly.
Actually, we received a real problem on it (like crbug.com/425563), so it's time
to fix it.
I'd like to make a change on SO_BROADCAST in net/ layer,
rather than re-binding hack workaround in PPAPI or ARC layer, because;
1) It does not leave a pit hole bug/trap in setsockopt implementation.
2) It can be simply implemented on among platforms, incl. Win, Mac, Linux and
Chrome OS.
(Though current our focus is Chrome OS). I.e., it can be provided as a
platform independent API.
3) Its semantics is simple and understandable. The user of UDPSocket can call
SetBroadcast anytime they wants,
and it works.
Thanks,
- hidehiko
> If we don't _have_to_ provide POSIX semantics, keeping the same mode for the
> lifetime of the socket is a little cleaner (IMHO). A pretty convincing counter
> argument would be performance degradation (we do almost anything for
> performance), but I don't think that applies in this case.
>
> Most likely what's going on is that I have no idea how important is to change
> the behavior.
On 2014/11/19 05:57:24, hidehiko wrote:
> On 2014/11/14 02:39:26, rvargas wrote:
> > On 2014/11/14 01:44:50, hidehiko wrote:
> >
> > I'm cleaning a little this thread to make the response more readable.
> >
> > > > That's an interesting way to put it. But you have not addressed why
> > "reserved"
> > > > is not strictly a subset of "stolen". Or to say it in another way,
anybody
> > who
> > > > is subject to the "stolen" use case needs to worry about the thief
> stealing
> > > the
> > > > port before the first bind (making the distinction between the two cases
> > > > irrelevant).
> > > >
> > > > - If there is no need to worry about stealing then there is no need to
> > change
> > > > the mode of operation in-flight.
> > > > - If there is a need to worry about stealing then there is no way to
> > securely
> > > > grab the port in the first place (or the same mechanism that copes with
it
> > can
> > > > be used when the port is gone).
> > > >
> > >
> > > Sorry, but I don't understand your meaning.
> > > Why "If there is no need to worry about stealing then there is no need to
> > change
> > > the mode of operation in-flight."?
> >
> > I'm just making the two cases explicit.
> >
> > What I mean is that if I'm a random app developer and I know that I don't
have
> > to worry about losing the port, then no changes to the current API are
needed.
> I
> > believe we agree here, so that means that this is a case that is not very
> > interesting to us for the purpose of this discussion.
> >
>
> At which time? On bind() or on setsockopt()? It is what I'm interested in.
setsockopt()? I thought we were talking about AllowBroadcast() :p
> If I were a developer, I'd probably imagine an error case on bind() (and write
> an error handling),
> but I would not think to loose it on setsockopt(). I.e., if setsockopt()
failed,
> I would imagine it is a missing feature or something, but I would not think
the
> port was stolen.
> I still think "cannot grab the port" and "the grabbed port is stolen" are
> different...
> Am I missing something?
I think the misunderstanding comes from thinking about a different starting
point. If I'm a dev, and I start from something that says "the mode can be
changed at anytime during the socket lifetime" then yes, I have no reason to
believe that I'm losing the socket when calling AllowBroadcast. On the other
hand, if I start from something that says "the mode is set for the whole
lifetime of the socket", and the way to change the mode is to use another socket
then that points me immediately to the same code that I had to use when calling
Bind().
This is why I said that the argument boils down to net having to provide POSIX
semantics.
>
> > > Changing the socket mode and if port will be stolen or not does not sound
> > > connected each other?
> > >
> > > Why "If there is a need to worry about stealing then there is no way to
> > securely
> > > grab the port in the first place (or the same mechanism that copes with it
> can
> > > be used when the port is gone)."?
> >
> > This is the case that is interesting... In this case, I know (as a random
app
> > developer), that I have to worry about port squatting (someone else
attempting
> > to use the port I want, at any time). In this case, closing the socket and
> > creating a new one presents an opportunity for that squatter to sit in the
> port
> > that I want. But that squatter can also do that before I grab the port for
the
> > first time.
> >
> > My question is why could we say that a squatter can grab the port while I
try
> to
> > change the mode but not before I create the first socket?
> > I don't see any reason to dismiss that scenario to be less likely, because
we
> > start from the assumption that there is someone trying to grab that port.
And
> > that means that I (as an app dev) have to write my app (protocol or
whatever)
> in
> > a way that I can deal properly with that case: not getting the port that I
> want
> > when I start, or not getting it when I switch mode.
> >
> > Covering one hole but leaving the other one open doesn't really help me.
> >
> > > There could be some period between the bind() and setsockopt().
> > > Then first bind() succeeds but then stolen?
> > >
> > > I'd appreciate more detailed explanation.
> >
> > > > > Surely, we are not necessary to be a thin wrapper of OS APIs,
> > > > > but we really need enough APIs for setsockopt compatible
implementation.
> > > > I'm glad you say that, but the only motivation that I see at this point
is
> > "we
> > > > want to promise POSIX semantics to our users" and that basically
> translates
> > in
> > > > net exposing a thin wrapper around the OS API :(
> > > >
> > >
> > > I also do not understand your motivation, then...
> > > POSIX semantics is the requirements for ARC.
> > > The new API incl. semantics looks simple.
> > > We do not need platform-depended hack for its implementation.
> > > What is the main problem here?
> >
> > What is ARC? When I read the linked bugs that was mentioned but that is far
> away
> > from src/net... which means that may motivate changes to PPAPI but not
> > necessarily to net (which is what I'm focusing on here).
> >
> > If we _have_to_ provide POSIX semantics then sure, let's change the mode at
> any
> > time.
> >
>
> Oh, sorry. Please let me share more backgrounds.
> I'm working on ARC (App Runtime for Chrome) project, which is to run android
> applications on Chrome, launched two months ago.
> In the project, we implement posix-layer on top of NaCl IRTs and PPAPIs.
> Specifically, we implement socket related posix functions on top of socket
> related PPAPIs.
So we are talking about taking code that was targeted to a platform and
emulating the expected behavior on another platform, right? That says to me that
PPAPI wants to use a POSIX version of a socket, which means that net should
export a POSIX version of a socket, right?
Which prompts two general questions:
1. What is the plan for platforms where the OS layer doesn't provide POSIX
behavior for sockets?
2. Why is net exposing that functionality at all? Why have some class in net
expose something called AllowBroadcast if what the callers want is something
that behaves like setsockopt on POSIX? Why not call setsockopt without involving
net code at all?
> {TCP,UDP}Socket::SetOption is the ones we use for setsockopt implementation.
> We are thinking that the compatibility is VERY important. I believe you agree
> that,
> "in this kind of project", incompatibility easily causes a nightmare bugs,
even
> if the
> incompatibility looks very small. Considering to increase the number of
> supported apps,
> its risk would be increased rapidly.
> Actually, we received a real problem on it (like crbug.com/425563), so it's
time
> to fix it.
>
> I'd like to make a change on SO_BROADCAST in net/ layer,
> rather than re-binding hack workaround in PPAPI or ARC layer, because;
> 1) It does not leave a pit hole bug/trap in setsockopt implementation.
> 2) It can be simply implemented on among platforms, incl. Win, Mac, Linux and
> Chrome OS.
> (Though current our focus is Chrome OS). I.e., it can be provided as a
> platform independent API.
> 3) Its semantics is simple and understandable. The user of UDPSocket can call
> SetBroadcast anytime they wants,
> and it works.
>
> Thanks,
> - hidehiko
>
> > If we don't _have_to_ provide POSIX semantics, keeping the same mode for the
> > lifetime of the socket is a little cleaner (IMHO). A pretty convincing
counter
> > argument would be performance degradation (we do almost anything for
> > performance), but I don't think that applies in this case.
> >
> > Most likely what's going on is that I have no idea how important is to
change
> > the behavior.
hidehiko@chromium.org changed reviewers: - jar@chromium.org, rsleevi@chromium.org, rvargas@chromium.org, sergeyu@chromium.org, shess@chromium.org
hidehiko@chromium.org changed required reviewers: - rvargas@chromium.org
Moved net/ reviewers from R= to CC=, because the net/ part are submitted separately. Bill, I re-implemented the change on ToT. Could you kindly take another look? Thanks, - hidehiko
https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:66: socket_options_(SOCKET_OPTION_NODELAY), Even though not strictly necessary in this case, we generally initialize all fields, so initialize rcvbuf_size and sndbuf_size here. Here and below. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:48: : socket_options_(0), Even though not strictly necessary in this case, we generally initialize all fields, so initialize rcvbuf_size and sndbuf_size here. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:121: // PPAPI tries to provide a platform independent APIs. s/the Windows/Windows s/a platform/platform https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:145: // UDPSocket instance is not yet created. So remember the value here. nit: s/. So/, so https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:167: // UDPSocket instance is not yet created. So remember the value here. nit: s/. So/, so https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:186: // UDPSocket instance is not yet created. So remember the value here. nit: s/. So/, so https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:23: * This option can only be set before calling <code>Bind()</code>. Looking at the UDP resource base implementation, ADDRESS_REUSE and BROADCAST are checked in the same way, but the documentation here says they're different. Which is correct? https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:38: * On version 1.0, this option can only be set before calling Option can be set only *after* Bind(). And below. https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:72: case PP_UDPSOCKET_OPTION_ADDRESS_REUSE: In the UDP IDL file, the documentation doesn't say ADDRESS_REUSE can be set after Bind() in version 1.1, but here it appears to not cause an error. https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: if (check_bind_state && bind_called_) You should add a comment about *why* you use bind_called_ here instead of bound_. https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:111: // |bind_called_| is true if Bind() is called, while |bound_| is true, nit: s/if /after I think the comment would be slightly easier to read that way.
Thank you for review! PTAL. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:66: socket_options_(SOCKET_OPTION_NODELAY), On 2014/12/09 18:37:37, bbudge wrote: > Even though not strictly necessary in this case, we generally initialize all > fields, so initialize rcvbuf_size and sndbuf_size here. > Here and below. Done. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:48: : socket_options_(0), On 2014/12/09 18:37:37, bbudge wrote: > Even though not strictly necessary in this case, we generally initialize all > fields, so initialize rcvbuf_size and sndbuf_size here. Done. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:121: // PPAPI tries to provide a platform independent APIs. On 2014/12/09 18:37:37, bbudge wrote: > s/the Windows/Windows > s/a platform/platform Done. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:145: // UDPSocket instance is not yet created. So remember the value here. On 2014/12/09 18:37:37, bbudge wrote: > nit: s/. So/, so Done. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:167: // UDPSocket instance is not yet created. So remember the value here. On 2014/12/09 18:37:37, bbudge wrote: > nit: s/. So/, so Done. https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:186: // UDPSocket instance is not yet created. So remember the value here. On 2014/12/09 18:37:37, bbudge wrote: > nit: s/. So/, so Done. https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:23: * This option can only be set before calling <code>Bind()</code>. On 2014/12/09 18:37:38, bbudge wrote: > Looking at the UDP resource base implementation, ADDRESS_REUSE and BROADCAST are > checked in the same way, but the documentation here says they're different. > Which is correct? The comment is correct. Please see my reply in the source file, too. https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:38: * On version 1.0, this option can only be set before calling On 2014/12/09 18:37:38, bbudge wrote: > Option can be set only *after* Bind(). > And below. Oops, good catch. I wrongly updated the comment. Reverted to the original sentences. https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:72: case PP_UDPSOCKET_OPTION_ADDRESS_REUSE: On 2014/12/09 18:37:38, bbudge wrote: > In the UDP IDL file, the documentation doesn't say ADDRESS_REUSE can be set > after Bind() in version 1.1, but here it appears to not cause an error. First, I thought it was not necessary, because message filter rejects it. However, I was wrong, and I should return an error here to provide stable behavior, because the message filter's behavior depends on the Bind() result. https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: if (check_bind_state && bind_called_) On 2014/12/09 18:37:38, bbudge wrote: > You should add a comment about *why* you use bind_called_ here instead of > bound_. Done. https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:111: // |bind_called_| is true if Bind() is called, while |bound_| is true, On 2014/12/09 18:37:38, bbudge wrote: > nit: s/if /after > I think the comment would be slightly easier to read that way. Done.
On 2014/12/09 19:51:42, hidehiko wrote: > Thank you for review! PTAL. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc > (right): > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:66: > socket_options_(SOCKET_OPTION_NODELAY), > On 2014/12/09 18:37:37, bbudge wrote: > > Even though not strictly necessary in this case, we generally initialize all > > fields, so initialize rcvbuf_size and sndbuf_size here. > > Here and below. > > Done. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc > (right): > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:48: : > socket_options_(0), > On 2014/12/09 18:37:37, bbudge wrote: > > Even though not strictly necessary in this case, we generally initialize all > > fields, so initialize rcvbuf_size and sndbuf_size here. > > Done. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:121: // > PPAPI tries to provide a platform independent APIs. > On 2014/12/09 18:37:37, bbudge wrote: > > s/the Windows/Windows > > s/a platform/platform > > Done. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:145: // > UDPSocket instance is not yet created. So remember the value here. > On 2014/12/09 18:37:37, bbudge wrote: > > nit: s/. So/, so > > Done. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:167: // > UDPSocket instance is not yet created. So remember the value here. > On 2014/12/09 18:37:37, bbudge wrote: > > nit: s/. So/, so > > Done. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:186: // > UDPSocket instance is not yet created. So remember the value here. > On 2014/12/09 18:37:37, bbudge wrote: > > nit: s/. So/, so > > Done. > > https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... > File ppapi/api/ppb_udp_socket.idl (right): > > https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... > ppapi/api/ppb_udp_socket.idl:23: * This option can only be set before calling > <code>Bind()</code>. > On 2014/12/09 18:37:38, bbudge wrote: > > Looking at the UDP resource base implementation, ADDRESS_REUSE and BROADCAST > are > > checked in the same way, but the documentation here says they're different. > > Which is correct? > > The comment is correct. Please see my reply in the source file, too. > > https://codereview.chromium.org/690903002/diff/300001/ppapi/api/ppb_udp_socke... > ppapi/api/ppb_udp_socket.idl:38: * On version 1.0, this option can only be set > before calling > On 2014/12/09 18:37:38, bbudge wrote: > > Option can be set only *after* Bind(). > > And below. > > Oops, good catch. I wrongly updated the comment. Reverted to the original > sentences. > > https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... > File ppapi/proxy/udp_socket_resource_base.cc (right): > > https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... > ppapi/proxy/udp_socket_resource_base.cc:72: case > PP_UDPSOCKET_OPTION_ADDRESS_REUSE: > On 2014/12/09 18:37:38, bbudge wrote: > > In the UDP IDL file, the documentation doesn't say ADDRESS_REUSE can be set > > after Bind() in version 1.1, but here it appears to not cause an error. > > First, I thought it was not necessary, because message filter rejects it. > However, I was wrong, and I should return an error here to provide stable > behavior, because the message filter's behavior depends on the Bind() result. > > https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... > ppapi/proxy/udp_socket_resource_base.cc:74: if (check_bind_state && > bind_called_) > On 2014/12/09 18:37:38, bbudge wrote: > > You should add a comment about *why* you use bind_called_ here instead of > > bound_. > > Done. > > https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... > File ppapi/proxy/udp_socket_resource_base.h (right): > > https://codereview.chromium.org/690903002/diff/300001/ppapi/proxy/udp_socket_... > ppapi/proxy/udp_socket_resource_base.h:111: // |bind_called_| is true if Bind() > is called, while |bound_| is true, > On 2014/12/09 18:37:38, bbudge wrote: > > nit: s/if /after > > I think the comment would be slightly easier to read that way. > > Done. Bill, friendly ping?
LGTM w comment https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:80: // successful Bind() call, so it's too late. OK, I think I understand the issue. How about something like this comment? // SetOption should fail in this case in order to give predictable behavior // while binding. Note that we use |bind_called_| rather than |bound_| since // the latter is only set on successful completion of Bind().
Thank you for review. Submitting. https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:80: // successful Bind() call, so it's too late. On 2014/12/11 02:49:06, bbudge wrote: > OK, I think I understand the issue. How about something like this comment? > // SetOption should fail in this case in order to give predictable behavior > // while binding. Note that we use |bind_called_| rather than |bound_| since > // the latter is only set on successful completion of Bind(). Done.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690903002/340001
Message was sent while issue was closed.
Committed patchset #15 (id:340001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/fd305c12b3fe8b4d2496f67aed089ff703f19b6d Cr-Commit-Position: refs/heads/master@{#307867} |
