Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
494573002:
A change for the setPause() api in chrome.sockets.tcp:
|
Created:
6 years, 4 months ago by lally Modified:
4 years, 11 months ago CC:
cbentzel+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, ldixon Base URL:
svn://svn.chromium.org/chrome/trunk/src Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionA change for the setPause() api in chrome.sockets.tcp:
- setPause's callback will not be invoked until after any pending
onReceive callbacks have already been invoked.
- No onReceive callbacks will be invoked until setPause(false) is
invoked after a successful setPause(true).
BUG=403076
Patch Set 1 #Patch Set 2 : Substantially cleaned up, and verified to make secure() work in STARTLS cases. #Patch Set 3 : Updated to pause TLS sockets and a substantial refactor. #Patch Set 4 : Cosmetics and commentary. #
Total comments: 15
Messages
Total messages: 33 (3 generated)
I am adding rsleevi@ for his advice on this issue. I am wondering is there could be a way to make this change simpler. For example, would it be reasonable to try to cancel the read operations at the OS level instead of simulating/buffering in user code?
I was hoping to have it a little better "cooked" before Ryan saw it, but that's fine. Unless there's a buffering layer below that I don't know about, canceling syscalls has traditionally been quite problematic. Cheers, $ ls On Aug 22, 2014 1:50 PM, <rpaquay@chromium.org> wrote: > I am adding rsleevi@ for his advice on this issue. I am wondering is > there could > be a way to make this change simpler. > > For example, would it be reasonable to try to cancel the read operations > at the > OS level instead of simulating/buffering in user code? > > > https://codereview.chromium.org/494573002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/22 19:02:50, chromium-reviews wrote: > I was hoping to have it a little better "cooked" before Ryan saw it, but > that's fine. Oh, sorry about that. > Unless there's a buffering layer below that I don't know about, canceling > syscalls has traditionally been quite problematic. I see. I thought it was possible on Windows, but I am not too familiar with Linux and MacOS. Another option: I was thinking maybe a way to make this CL simpler and avoid exposing an additional "buffering" layers would be to do the buffering in the "tcp_socket_event_dispatcher". The implementation should have all knowledge needed to perform the buffering, and it would need to buffer only one request as it is the "controller" that decides went to emit new recv calls. If there was just an additional TCPSocketEventDispatcher::OnSocketPaused entry, i think all the logic could remain in that class. WDYT?
On 2014/08/22 19:18:57, rpaquay wrote: > On 2014/08/22 19:02:50, chromium-reviews wrote: > > I was hoping to have it a little better "cooked" before Ryan saw it, but > > that's fine. > > Oh, sorry about that. > > > > Unless there's a buffering layer below that I don't know about, canceling > > syscalls has traditionally been quite problematic. > > I see. I thought it was possible on Windows, but I am not too familiar with > Linux and MacOS. It's not, on any of our operating systems, nor is it a networking pattern we're willing to support. > > Another option: I was thinking maybe a way to make this CL simpler and > avoid exposing an additional "buffering" layers would be to do the > buffering in the "tcp_socket_event_dispatcher". The implementation > should have all knowledge needed to perform the buffering, and it > would need to buffer only one request as it is the "controller" that > decides went to emit new recv calls. If there was just an additional > TCPSocketEventDispatcher::OnSocketPaused entry, i think all the logic > could remain in that class. > > WDYT? In general, we're strongly opposed to any buffering in //net.
In general, I think I'm fairly strongly opposed to this API - or more importantly, what it's trying to accomplish. I know, I know, it's not quite ready - but it's better to engage early on with the features, rather than later. There's no BUG listed here, so it's hard to understand the problem you're trying to solve, and thus harder to offer you alternatives. I'd suggest you write up the issues first, then we can discuss, and if necessary, we can take it over to net-dev@ for this. However, buffering, delaying callbacks like this, these are all things we explicitly try to avoid as strong socket anti-patterns.
Fair enough. I've updated the BUG= in the description to point to the original bug (https://code.google.com/p/chromium/issues/detail?id=403076), and updated that bug with a write-up of what's going on behind the scenes. Apologies for the UML. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Aug 25, 2014 at 12:23 AM, <rsleevi@chromium.org> wrote: > In general, I think I'm fairly strongly opposed to this API - or more > importantly, what it's trying to accomplish. > > I know, I know, it's not quite ready - but it's better to engage early on > with > the features, rather than later. > > There's no BUG listed here, so it's hard to understand the problem you're > trying > to solve, and thus harder to offer you alternatives. I'd suggest you write > up > the issues first, then we can discuss, and if necessary, we can take it > over to > net-dev@ for this. > > However, buffering, delaying callbacks like this, these are all things we > explicitly try to avoid as strong socket anti-patterns. > > https://codereview.chromium.org/494573002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from
==========
A change for the setPause() api in chrome.sockets.tcp:
- setPause's callback will not be invoked until after any pending
onReceive callbacks have already been invoked.
- No onReceive callbacks will be invoked until setPause(false) is
invoked after a successful setPause(true).
Needs tests, and the same implementation on server sockets.
BUG=403076
==========
to
==========
A change for the setPause() api in chrome.sockets.tcp:
- setPause's callback will not be invoked until after any pending
onReceive callbacks have already been invoked.
- No onReceive callbacks will be invoked until setPause(false) is
invoked after a successful setPause(true).
BUG=403076
==========
lally@chromium.org changed reviewers: - rsleevi@chromium.org
lally@chromium.org changed reviewers: + jam@chromium.org, rockot@chromium.org
Hello, this is a fix to sockets.tcp's setPause() implementation. setPause() has a TCPSocketEventDispatcher, which issues Read()s on the socket, and then issues onReceive events back to JS when those Read()s complete. But, when pausing, any issued Read() continues to live on. Currently, that means that another onReceive() is likely to come, even when the socket's paused. This change buffers the data that comes back from Read() for sending later, when the socket is un-paused. I've written up the issues here: https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/. We don't do backpressure here (that's in a follow-up CL), but some of the testing support needed for that is included here. Please take a look, and let me know if you have any questions. I'm happy to talk about this. I spoke to rsleevi@ privately, and he's not interested in reviewing this CL.
On 2015/12/04 at 19:32:40, lally wrote: > Hello, this is a fix to sockets.tcp's setPause() implementation. setPause() has a TCPSocketEventDispatcher, which issues Read()s on the socket, and then issues onReceive events back to JS when those Read()s complete. > > But, when pausing, any issued Read() continues to live on. Currently, that means that another onReceive() is likely to come, even when the socket's paused. This change buffers the data that comes back from Read() for sending later, when the socket is un-paused. > > I've written up the issues here: https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/. We don't do backpressure here (that's in a follow-up CL), but some of the testing support needed for that is included here. > > > Please take a look, and let me know if you have any questions. I'm happy to talk about this. > > I spoke to rsleevi@ privately, and he's not interested in reviewing this CL. This is an unfortunate problem and I agree that we need to fix it, but this solution seems fairly high in complexity to me. Here is my counter-proposal for what we might do: 1. Add a read() API that works in a new "manual mode" only 2. Add an option on socket creation to create the socket in "manual mode" - i.e. no auto-polling for receive 3. Add an API to enable automatic receive polling (i.e. to leave "manual mode"). NO API for switching back to manual mode, because this is unnecessary and a source of much complexity here. Separately we could also address two (IMHO) lower priority issues: 4. Defer the setPaused callback until the receive queue is flushed, just because this feels like the right thing to do. 5. Add an API option to throttle reads in auto-poll mode, to avoid the over-eager reading problem. I think adding 1-3 would be lower complexity than what you have here, AFAICT it would address all of your needs, and it doesn't actually change any behavior of existing API uses in the wild (for better or worse). #4 is nice to have, #5 seems to me like a more flexible throttling solution than strictly tying read call frequency to the JS event loop. WDYT?
On 2015/12/07 at 15:54:29, Ken Rockot wrote: > On 2015/12/04 at 19:32:40, lally wrote: > > Hello, this is a fix to sockets.tcp's setPause() implementation. setPause() has a TCPSocketEventDispatcher, which issues Read()s on the socket, and then issues onReceive events back to JS when those Read()s complete. > > > > But, when pausing, any issued Read() continues to live on. Currently, that means that another onReceive() is likely to come, even when the socket's paused. This change buffers the data that comes back from Read() for sending later, when the socket is un-paused. > > > > I've written up the issues here: https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/. We don't do backpressure here (that's in a follow-up CL), but some of the testing support needed for that is included here. > > > > > > Please take a look, and let me know if you have any questions. I'm happy to talk about this. > > > > I spoke to rsleevi@ privately, and he's not interested in reviewing this CL. > > This is an unfortunate problem and I agree that we need to fix it, but this solution seems fairly high in complexity to me. Here is my counter-proposal for what we might do: > > 1. Add a read() API that works in a new "manual mode" only > 2. Add an option on socket creation to create the socket in "manual mode" - i.e. no auto-polling for receive > 3. Add an API to enable automatic receive polling (i.e. to leave "manual mode"). NO API for switching back to manual mode, because this is unnecessary and a source of much complexity here. > > Separately we could also address two (IMHO) lower priority issues: > > 4. Defer the setPaused callback until the receive queue is flushed, just because this feels like the right thing to do. > 5. Add an API option to throttle reads in auto-poll mode, to avoid the over-eager reading problem. > > I think adding 1-3 would be lower complexity than what you have here, AFAICT it would address all of your needs, and it doesn't actually change any behavior of existing API uses in the wild (for better or worse). > > #4 is nice to have, #5 seems to me like a more flexible throttling solution than strictly tying read call frequency to the JS event loop. > > WDYT? Or maybe instead of "manual mode" which is a bit confusing, we have a "start paused" option. Then read() is, as you suggest, a way of reading from the socket while paused. The main difference is that we can avoid trying to resolve the complexity of pausing at a very specific place in the stream, by making it possible to start the socket in a paused state. The trade-off is that you have to do manual reads until you're ready to initiate TLS and unpause, but that doesn't seem so bad to me.
I'm happy with a "start paused" option, and a read() method to access a paused stream. I think that's a natural extension of the current API. For #4: The problem with just deferring the setPaused() callback is that data may not be coming back anytime soon, so that callback can sit and wait a long time. I'm not sure how to get around that without buffering. For #5: any thoughts on what kind of throttle specification that'd be? A max-outstanding-JS events count, where you can specify a count > 1, can keep throughput high. But if we stop talking in terms of outstanding JS events, we have to find another way to maintain sensitivity to CPU pressure. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: > On 2015/12/07 at 15:54:29, Ken Rockot wrote: > >> On 2015/12/04 at 19:32:40, lally wrote: >> > Hello, this is a fix to sockets.tcp's setPause() implementation. >> setPause() >> > has a TCPSocketEventDispatcher, which issues Read()s on the socket, and > then > issues onReceive events back to JS when those Read()s complete. > >> > >> > But, when pausing, any issued Read() continues to live on. Currently, >> that >> > means that another onReceive() is likely to come, even when the socket's > paused. > This change buffers the data that comes back from Read() for sending > later, > when the socket is un-paused. > >> > >> > I've written up the issues here: >> > > https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ > . > We don't do backpressure here (that's in a follow-up CL), but some of the > testing support needed for that is included here. > >> > >> > >> > Please take a look, and let me know if you have any questions. I'm >> happy to >> > talk about this. > >> > >> > I spoke to rsleevi@ privately, and he's not interested in reviewing >> this CL. >> > > This is an unfortunate problem and I agree that we need to fix it, but this >> > solution seems fairly high in complexity to me. Here is my > counter-proposal for > what we might do: > > 1. Add a read() API that works in a new "manual mode" only >> 2. Add an option on socket creation to create the socket in "manual >> mode" - >> > i.e. no auto-polling for receive > >> 3. Add an API to enable automatic receive polling (i.e. to leave >> "manual >> > mode"). NO API for switching back to manual mode, because this is > unnecessary > and a source of much complexity here. > > Separately we could also address two (IMHO) lower priority issues: >> > > 4. Defer the setPaused callback until the receive queue is flushed, just >> > because this feels like the right thing to do. > >> 5. Add an API option to throttle reads in auto-poll mode, to avoid the >> > over-eager reading problem. > > I think adding 1-3 would be lower complexity than what you have here, >> AFAICT >> > it would address all of your needs, and it doesn't actually change any > behavior > of existing API uses in the wild (for better or worse). > > #4 is nice to have, #5 seems to me like a more flexible throttling solution >> > than strictly tying read call frequency to the JS event loop. > > WDYT? >> > > Or maybe instead of "manual mode" which is a bit confusing, we have a > "start > paused" option. Then read() is, as you suggest, a way of reading from the > socket > while paused. The main difference is that we can avoid trying to resolve > the > complexity of pausing at a very specific place in the stream, by making it > possible to start the socket in a paused state. > > The trade-off is that you have to do manual reads until you're ready to > initiate > TLS and unpause, but that doesn't seem so bad to me. > > > https://codereview.chromium.org/494573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think it would be reasonable (for #5) to have a max pending receive option. It would work generally as you describe (don't read until event(s) have been acked by JS), it would just support N > 1 outstanding receive events. Re #4: Doh... right, I see, because we can't just cancel an outstanding read syscall. In that case I'd like to just continue on a path that doesn't require us to solve that problem, which I think start-paused does. :) On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> wrote: > I'm happy with a "start paused" option, and a read() method to access a > paused stream. I think that's a natural extension of the current API. > > For #4: The problem with just deferring the setPaused() callback is that > data may not be coming back anytime soon, so that callback can sit and wait > a long time. I'm not sure how to get around that without buffering. > > For #5: any thoughts on what kind of throttle specification that'd be? A > max-outstanding-JS events count, where you can specify a count > 1, can > keep throughput high. But if we stop talking in terms of outstanding JS > events, we have to find another way to maintain sensitivity to CPU pressure. > > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: > >> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >> >>> On 2015/12/04 at 19:32:40, lally wrote: >>> > Hello, this is a fix to sockets.tcp's setPause() implementation. >>> setPause() >>> >> has a TCPSocketEventDispatcher, which issues Read()s on the socket, and >> then >> issues onReceive events back to JS when those Read()s complete. >> >>> > >>> > But, when pausing, any issued Read() continues to live on. Currently, >>> that >>> >> means that another onReceive() is likely to come, even when the socket's >> paused. >> This change buffers the data that comes back from Read() for sending >> later, >> when the socket is un-paused. >> >>> > >>> > I've written up the issues here: >>> >> >> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >> . >> We don't do backpressure here (that's in a follow-up CL), but some of the >> testing support needed for that is included here. >> >>> > >>> > >>> > Please take a look, and let me know if you have any questions. I'm >>> happy to >>> >> talk about this. >> >>> > >>> > I spoke to rsleevi@ privately, and he's not interested in reviewing >>> this CL. >>> >> >> This is an unfortunate problem and I agree that we need to fix it, but >>> this >>> >> solution seems fairly high in complexity to me. Here is my >> counter-proposal for >> what we might do: >> >> 1. Add a read() API that works in a new "manual mode" only >>> 2. Add an option on socket creation to create the socket in "manual >>> mode" - >>> >> i.e. no auto-polling for receive >> >>> 3. Add an API to enable automatic receive polling (i.e. to leave >>> "manual >>> >> mode"). NO API for switching back to manual mode, because this is >> unnecessary >> and a source of much complexity here. >> >> Separately we could also address two (IMHO) lower priority issues: >>> >> >> 4. Defer the setPaused callback until the receive queue is flushed, >>> just >>> >> because this feels like the right thing to do. >> >>> 5. Add an API option to throttle reads in auto-poll mode, to avoid the >>> >> over-eager reading problem. >> >> I think adding 1-3 would be lower complexity than what you have here, >>> AFAICT >>> >> it would address all of your needs, and it doesn't actually change any >> behavior >> of existing API uses in the wild (for better or worse). >> >> #4 is nice to have, #5 seems to me like a more flexible throttling >>> solution >>> >> than strictly tying read call frequency to the JS event loop. >> >> WDYT? >>> >> >> Or maybe instead of "manual mode" which is a bit confusing, we have a >> "start >> paused" option. Then read() is, as you suggest, a way of reading from the >> socket >> while paused. The main difference is that we can avoid trying to resolve >> the >> complexity of pausing at a very specific place in the stream, by making it >> possible to start the socket in a paused state. >> >> The trade-off is that you have to do manual reads until you're ready to >> initiate >> TLS and unpause, but that doesn't seem so bad to me. >> >> >> https://codereview.chromium.org/494573002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm happy with an N > 1 approach. I'm not sure how else to fix the currently-broken setPaused() api other than buffering. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> wrote: > I think it would be reasonable (for #5) to have a max pending receive > option. It would work generally as you describe (don't read until event(s) > have been acked by JS), it would just support N > 1 outstanding receive > events. > > Re #4: Doh... right, I see, because we can't just cancel an outstanding > read syscall. In that case I'd like to just continue on a path that doesn't > require us to solve that problem, which I think start-paused does. :) > > On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> wrote: > >> I'm happy with a "start paused" option, and a read() method to access a >> paused stream. I think that's a natural extension of the current API. >> >> For #4: The problem with just deferring the setPaused() callback is that >> data may not be coming back anytime soon, so that callback can sit and wait >> a long time. I'm not sure how to get around that without buffering. >> >> For #5: any thoughts on what kind of throttle specification that'd be? A >> max-outstanding-JS events count, where you can specify a count > 1, can >> keep throughput high. But if we stop talking in terms of outstanding JS >> events, we have to find another way to maintain sensitivity to CPU pressure. >> >> >> >> Cheers, >> $ ls >> >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | >> lally@google.com >> >> >> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >> >>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>> >>>> On 2015/12/04 at 19:32:40, lally wrote: >>>> > Hello, this is a fix to sockets.tcp's setPause() implementation. >>>> setPause() >>>> >>> has a TCPSocketEventDispatcher, which issues Read()s on the socket, and >>> then >>> issues onReceive events back to JS when those Read()s complete. >>> >>>> > >>>> > But, when pausing, any issued Read() continues to live on. >>>> Currently, that >>>> >>> means that another onReceive() is likely to come, even when the socket's >>> paused. >>> This change buffers the data that comes back from Read() for sending >>> later, >>> when the socket is un-paused. >>> >>>> > >>>> > I've written up the issues here: >>>> >>> >>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>> . >>> We don't do backpressure here (that's in a follow-up CL), but some of >>> the >>> testing support needed for that is included here. >>> >>>> > >>>> > >>>> > Please take a look, and let me know if you have any questions. I'm >>>> happy to >>>> >>> talk about this. >>> >>>> > >>>> > I spoke to rsleevi@ privately, and he's not interested in reviewing >>>> this CL. >>>> >>> >>> This is an unfortunate problem and I agree that we need to fix it, but >>>> this >>>> >>> solution seems fairly high in complexity to me. Here is my >>> counter-proposal for >>> what we might do: >>> >>> 1. Add a read() API that works in a new "manual mode" only >>>> 2. Add an option on socket creation to create the socket in "manual >>>> mode" - >>>> >>> i.e. no auto-polling for receive >>> >>>> 3. Add an API to enable automatic receive polling (i.e. to leave >>>> "manual >>>> >>> mode"). NO API for switching back to manual mode, because this is >>> unnecessary >>> and a source of much complexity here. >>> >>> Separately we could also address two (IMHO) lower priority issues: >>>> >>> >>> 4. Defer the setPaused callback until the receive queue is flushed, >>>> just >>>> >>> because this feels like the right thing to do. >>> >>>> 5. Add an API option to throttle reads in auto-poll mode, to avoid >>>> the >>>> >>> over-eager reading problem. >>> >>> I think adding 1-3 would be lower complexity than what you have here, >>>> AFAICT >>>> >>> it would address all of your needs, and it doesn't actually change any >>> behavior >>> of existing API uses in the wild (for better or worse). >>> >>> #4 is nice to have, #5 seems to me like a more flexible throttling >>>> solution >>>> >>> than strictly tying read call frequency to the JS event loop. >>> >>> WDYT? >>>> >>> >>> Or maybe instead of "manual mode" which is a bit confusing, we have a >>> "start >>> paused" option. Then read() is, as you suggest, a way of reading from >>> the socket >>> while paused. The main difference is that we can avoid trying to resolve >>> the >>> complexity of pausing at a very specific place in the stream, by making >>> it >>> possible to start the socket in a paused state. >>> >>> The trade-off is that you have to do manual reads until you're ready to >>> initiate >>> TLS and unpause, but that doesn't seem so bad to me. >>> >>> >>> https://codereview.chromium.org/494573002/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm not convinced it needs to be fixed as long as it's understood that there's no guarantee that data won't arrive after pause. I can imagine cases where it's still useful without that guarantee, but your case isn't one of them. On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> wrote: > I'm happy with an N > 1 approach. > > I'm not sure how else to fix the currently-broken setPaused() api other > than buffering. > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> wrote: > >> I think it would be reasonable (for #5) to have a max pending receive >> option. It would work generally as you describe (don't read until event(s) >> have been acked by JS), it would just support N > 1 outstanding receive >> events. >> >> Re #4: Doh... right, I see, because we can't just cancel an outstanding >> read syscall. In that case I'd like to just continue on a path that doesn't >> require us to solve that problem, which I think start-paused does. :) >> >> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> wrote: >> >>> I'm happy with a "start paused" option, and a read() method to access a >>> paused stream. I think that's a natural extension of the current API. >>> >>> For #4: The problem with just deferring the setPaused() callback is that >>> data may not be coming back anytime soon, so that callback can sit and wait >>> a long time. I'm not sure how to get around that without buffering. >>> >>> For #5: any thoughts on what kind of throttle specification that'd be? >>> A max-outstanding-JS events count, where you can specify a count > 1, can >>> keep throughput high. But if we stop talking in terms of outstanding JS >>> events, we have to find another way to maintain sensitivity to CPU pressure. >>> >>> >>> >>> Cheers, >>> $ ls >>> >>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>> Engineering | lally@google.com >>> >>> >>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >>> >>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>> >>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>>> > Hello, this is a fix to sockets.tcp's setPause() implementation. >>>>> setPause() >>>>> >>>> has a TCPSocketEventDispatcher, which issues Read()s on the socket, and >>>> then >>>> issues onReceive events back to JS when those Read()s complete. >>>> >>>>> > >>>>> > But, when pausing, any issued Read() continues to live on. >>>>> Currently, that >>>>> >>>> means that another onReceive() is likely to come, even when the >>>> socket's paused. >>>> This change buffers the data that comes back from Read() for sending >>>> later, >>>> when the socket is un-paused. >>>> >>>>> > >>>>> > I've written up the issues here: >>>>> >>>> >>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>> . >>>> We don't do backpressure here (that's in a follow-up CL), but some of >>>> the >>>> testing support needed for that is included here. >>>> >>>>> > >>>>> > >>>>> > Please take a look, and let me know if you have any questions. I'm >>>>> happy to >>>>> >>>> talk about this. >>>> >>>>> > >>>>> > I spoke to rsleevi@ privately, and he's not interested in reviewing >>>>> this CL. >>>>> >>>> >>>> This is an unfortunate problem and I agree that we need to fix it, but >>>>> this >>>>> >>>> solution seems fairly high in complexity to me. Here is my >>>> counter-proposal for >>>> what we might do: >>>> >>>> 1. Add a read() API that works in a new "manual mode" only >>>>> 2. Add an option on socket creation to create the socket in "manual >>>>> mode" - >>>>> >>>> i.e. no auto-polling for receive >>>> >>>>> 3. Add an API to enable automatic receive polling (i.e. to leave >>>>> "manual >>>>> >>>> mode"). NO API for switching back to manual mode, because this is >>>> unnecessary >>>> and a source of much complexity here. >>>> >>>> Separately we could also address two (IMHO) lower priority issues: >>>>> >>>> >>>> 4. Defer the setPaused callback until the receive queue is flushed, >>>>> just >>>>> >>>> because this feels like the right thing to do. >>>> >>>>> 5. Add an API option to throttle reads in auto-poll mode, to avoid >>>>> the >>>>> >>>> over-eager reading problem. >>>> >>>> I think adding 1-3 would be lower complexity than what you have here, >>>>> AFAICT >>>>> >>>> it would address all of your needs, and it doesn't actually change any >>>> behavior >>>> of existing API uses in the wild (for better or worse). >>>> >>>> #4 is nice to have, #5 seems to me like a more flexible throttling >>>>> solution >>>>> >>>> than strictly tying read call frequency to the JS event loop. >>>> >>>> WDYT? >>>>> >>>> >>>> Or maybe instead of "manual mode" which is a bit confusing, we have a >>>> "start >>>> paused" option. Then read() is, as you suggest, a way of reading from >>>> the socket >>>> while paused. The main difference is that we can avoid trying to >>>> resolve the >>>> complexity of pausing at a very specific place in the stream, by making >>>> it >>>> possible to start the socket in a paused state. >>>> >>>> The trade-off is that you have to do manual reads until you're ready to >>>> initiate >>>> TLS and unpause, but that doesn't seem so bad to me. >>>> >>>> >>>> https://codereview.chromium.org/494573002/ >>>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
In avoiding this, I think we'd just be making clients implement the same buffer in their code. Which is sad, because the problem is ours and we're doing a poor job here. Node.js implements a similar pause API and does a better job here. This CL is a buffer, some plumbing, and test cases. If it's too big or there are ways you can see to make it smaller, I'm happy to break it up or adjust it. We (uProxy) use the socket api quite a bit, and implement quite a bit atop: a SOCKS5 server, an XHR client, and an XMPP client. We've been hitting issues with sockets (basically, pausing and backpressure) for a while, and I'm quite eager to fix them. The workarounds in code are quite painful and cost us substantial amounts of memory and performance. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> wrote: > I'm not convinced it needs to be fixed as long as it's understood that > there's no guarantee that data won't arrive after pause. I can imagine > cases where it's still useful without that guarantee, but your case isn't > one of them. > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> wrote: > >> I'm happy with an N > 1 approach. >> >> I'm not sure how else to fix the currently-broken setPaused() api other >> than buffering. >> >> Cheers, >> $ ls >> >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | >> lally@google.com >> >> >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> wrote: >> >>> I think it would be reasonable (for #5) to have a max pending receive >>> option. It would work generally as you describe (don't read until event(s) >>> have been acked by JS), it would just support N > 1 outstanding receive >>> events. >>> >>> Re #4: Doh... right, I see, because we can't just cancel an outstanding >>> read syscall. In that case I'd like to just continue on a path that doesn't >>> require us to solve that problem, which I think start-paused does. :) >>> >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> wrote: >>> >>>> I'm happy with a "start paused" option, and a read() method to access a >>>> paused stream. I think that's a natural extension of the current API. >>>> >>>> For #4: The problem with just deferring the setPaused() callback is >>>> that data may not be coming back anytime soon, so that callback can sit and >>>> wait a long time. I'm not sure how to get around that without buffering. >>>> >>>> For #5: any thoughts on what kind of throttle specification that'd be? >>>> A max-outstanding-JS events count, where you can specify a count > 1, can >>>> keep throughput high. But if we stop talking in terms of outstanding JS >>>> events, we have to find another way to maintain sensitivity to CPU pressure. >>>> >>>> >>>> >>>> Cheers, >>>> $ ls >>>> >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>> Engineering | lally@google.com >>>> >>>> >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >>>> >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>>> >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>>>> > Hello, this is a fix to sockets.tcp's setPause() implementation. >>>>>> setPause() >>>>>> >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the socket, >>>>> and then >>>>> issues onReceive events back to JS when those Read()s complete. >>>>> >>>>>> > >>>>>> > But, when pausing, any issued Read() continues to live on. >>>>>> Currently, that >>>>>> >>>>> means that another onReceive() is likely to come, even when the >>>>> socket's paused. >>>>> This change buffers the data that comes back from Read() for sending >>>>> later, >>>>> when the socket is un-paused. >>>>> >>>>>> > >>>>>> > I've written up the issues here: >>>>>> >>>>> >>>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>>> . >>>>> We don't do backpressure here (that's in a follow-up CL), but some of >>>>> the >>>>> testing support needed for that is included here. >>>>> >>>>>> > >>>>>> > >>>>>> > Please take a look, and let me know if you have any questions. I'm >>>>>> happy to >>>>>> >>>>> talk about this. >>>>> >>>>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in >>>>>> reviewing this CL. >>>>>> >>>>> >>>>> This is an unfortunate problem and I agree that we need to fix it, but >>>>>> this >>>>>> >>>>> solution seems fairly high in complexity to me. Here is my >>>>> counter-proposal for >>>>> what we might do: >>>>> >>>>> 1. Add a read() API that works in a new "manual mode" only >>>>>> 2. Add an option on socket creation to create the socket in >>>>>> "manual mode" - >>>>>> >>>>> i.e. no auto-polling for receive >>>>> >>>>>> 3. Add an API to enable automatic receive polling (i.e. to leave >>>>>> "manual >>>>>> >>>>> mode"). NO API for switching back to manual mode, because this is >>>>> unnecessary >>>>> and a source of much complexity here. >>>>> >>>>> Separately we could also address two (IMHO) lower priority issues: >>>>>> >>>>> >>>>> 4. Defer the setPaused callback until the receive queue is flushed, >>>>>> just >>>>>> >>>>> because this feels like the right thing to do. >>>>> >>>>>> 5. Add an API option to throttle reads in auto-poll mode, to avoid >>>>>> the >>>>>> >>>>> over-eager reading problem. >>>>> >>>>> I think adding 1-3 would be lower complexity than what you have here, >>>>>> AFAICT >>>>>> >>>>> it would address all of your needs, and it doesn't actually change any >>>>> behavior >>>>> of existing API uses in the wild (for better or worse). >>>>> >>>>> #4 is nice to have, #5 seems to me like a more flexible throttling >>>>>> solution >>>>>> >>>>> than strictly tying read call frequency to the JS event loop. >>>>> >>>>> WDYT? >>>>>> >>>>> >>>>> Or maybe instead of "manual mode" which is a bit confusing, we have a >>>>> "start >>>>> paused" option. Then read() is, as you suggest, a way of reading from >>>>> the socket >>>>> while paused. The main difference is that we can avoid trying to >>>>> resolve the >>>>> complexity of pausing at a very specific place in the stream, by >>>>> making it >>>>> possible to start the socket in a paused state. >>>>> >>>>> The trade-off is that you have to do manual reads until you're ready >>>>> to initiate >>>>> TLS and unpause, but that doesn't seem so bad to me. >>>>> >>>>> >>>>> https://codereview.chromium.org/494573002/ >>>>> >>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/07 at 20:12:49, chromium-reviews wrote: > In avoiding this, I think we'd just be making clients implement the same > buffer in their code. > > Which is sad, because the problem is ours and we're doing a poor job here. > Node.js implements a similar pause API and does a better job here. I would prefer our APIs remain minimalist in their design. Auto-polling at the implementation layer adds value in that it avoids some latency imposed by the JS event loop. I don't a similar level of value being added by this buffering behavior. Since pause can't actually stop a pending read from completing or data from arriving, this buffering only artificially hides an event from the client. Why is that better than firing the event and letting the client decide what to do? I'm certain that not every client is averse to receiving and processing some data after issuing a pause. > > This CL is a buffer, some plumbing, and test cases. If it's too big or > there are ways you can see to make it smaller, I'm happy to break it up or > adjust it. > > We (uProxy) use the socket api quite a bit, and implement quite a bit atop: > a SOCKS5 server, an XHR client, and an XMPP client. We've been hitting > issues with sockets (basically, pausing and backpressure) for a while, and > I'm quite eager to fix them. The workarounds in code are quite painful and > cost us substantial amounts of memory and performance. > > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> wrote: > > > I'm not convinced it needs to be fixed as long as it's understood that > > there's no guarantee that data won't arrive after pause. I can imagine > > cases where it's still useful without that guarantee, but your case isn't > > one of them. > > > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> wrote: > > > >> I'm happy with an N > 1 approach. > >> > >> I'm not sure how else to fix the currently-broken setPaused() api other > >> than buffering. > >> > >> Cheers, > >> $ ls > >> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > >> lally@google.com > >> > >> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> wrote: > >> > >>> I think it would be reasonable (for #5) to have a max pending receive > >>> option. It would work generally as you describe (don't read until event(s) > >>> have been acked by JS), it would just support N > 1 outstanding receive > >>> events. > >>> > >>> Re #4: Doh... right, I see, because we can't just cancel an outstanding > >>> read syscall. In that case I'd like to just continue on a path that doesn't > >>> require us to solve that problem, which I think start-paused does. :) > >>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> wrote: > >>> > >>>> I'm happy with a "start paused" option, and a read() method to access a > >>>> paused stream. I think that's a natural extension of the current API. > >>>> > >>>> For #4: The problem with just deferring the setPaused() callback is > >>>> that data may not be coming back anytime soon, so that callback can sit and > >>>> wait a long time. I'm not sure how to get around that without buffering. > >>>> > >>>> For #5: any thoughts on what kind of throttle specification that'd be? > >>>> A max-outstanding-JS events count, where you can specify a count > 1, can > >>>> keep throughput high. But if we stop talking in terms of outstanding JS > >>>> events, we have to find another way to maintain sensitivity to CPU pressure. > >>>> > >>>> > >>>> > >>>> Cheers, > >>>> $ ls > >>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | > >>>> Engineering | lally@google.com > >>>> > >>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: > >>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: > >>>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() implementation. > >>>>>> setPause() > >>>>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the socket, > >>>>> and then > >>>>> issues onReceive events back to JS when those Read()s complete. > >>>>> > >>>>>> > > >>>>>> > But, when pausing, any issued Read() continues to live on. > >>>>>> Currently, that > >>>>>> > >>>>> means that another onReceive() is likely to come, even when the > >>>>> socket's paused. > >>>>> This change buffers the data that comes back from Read() for sending > >>>>> later, > >>>>> when the socket is un-paused. > >>>>> > >>>>>> > > >>>>>> > I've written up the issues here: > >>>>>> > >>>>> > >>>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ > >>>>> . > >>>>> We don't do backpressure here (that's in a follow-up CL), but some of > >>>>> the > >>>>> testing support needed for that is included here. > >>>>> > >>>>>> > > >>>>>> > > >>>>>> > Please take a look, and let me know if you have any questions. I'm > >>>>>> happy to > >>>>>> > >>>>> talk about this. > >>>>> > >>>>>> > > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in > >>>>>> reviewing this CL. > >>>>>> > >>>>> > >>>>> This is an unfortunate problem and I agree that we need to fix it, but > >>>>>> this > >>>>>> > >>>>> solution seems fairly high in complexity to me. Here is my > >>>>> counter-proposal for > >>>>> what we might do: > >>>>> > >>>>> 1. Add a read() API that works in a new "manual mode" only > >>>>>> 2. Add an option on socket creation to create the socket in > >>>>>> "manual mode" - > >>>>>> > >>>>> i.e. no auto-polling for receive > >>>>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. to leave > >>>>>> "manual > >>>>>> > >>>>> mode"). NO API for switching back to manual mode, because this is > >>>>> unnecessary > >>>>> and a source of much complexity here. > >>>>> > >>>>> Separately we could also address two (IMHO) lower priority issues: > >>>>>> > >>>>> > >>>>> 4. Defer the setPaused callback until the receive queue is flushed, > >>>>>> just > >>>>>> > >>>>> because this feels like the right thing to do. > >>>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, to avoid > >>>>>> the > >>>>>> > >>>>> over-eager reading problem. > >>>>> > >>>>> I think adding 1-3 would be lower complexity than what you have here, > >>>>>> AFAICT > >>>>>> > >>>>> it would address all of your needs, and it doesn't actually change any > >>>>> behavior > >>>>> of existing API uses in the wild (for better or worse). > >>>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible throttling > >>>>>> solution > >>>>>> > >>>>> than strictly tying read call frequency to the JS event loop. > >>>>> > >>>>> WDYT? > >>>>>> > >>>>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, we have a > >>>>> "start > >>>>> paused" option. Then read() is, as you suggest, a way of reading from > >>>>> the socket > >>>>> while paused. The main difference is that we can avoid trying to > >>>>> resolve the > >>>>> complexity of pausing at a very specific place in the stream, by > >>>>> making it > >>>>> possible to start the socket in a paused state. > >>>>> > >>>>> The trade-off is that you have to do manual reads until you're ready > >>>>> to initiate > >>>>> TLS and unpause, but that doesn't seem so bad to me. > >>>>> > >>>>> > >>>>> https://codereview.chromium.org/494573002/ > >>>>> > >>>> > >>>> > >>> > >> > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
I believe Lally shared the the proposed design <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite a while ago and it was discussed a bunch then. My understanding is that its purpose is to fix two bad things with the current API: set-paused is broken, which means that many secure socket flows need to be hacked/don't work, and there is no back-pressure, which means you have to implement a very inefficient version in JS. Having back-pressure and secure sockets behave properly seems good to me, and is certainly what I'd expect of a tcp socket library. I suspect there is some basic misunderstanding here, maybe a short VC would help? On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: > On 2015/12/07 at 20:12:49, chromium-reviews wrote: > > In avoiding this, I think we'd just be making clients implement the same > > buffer in their code. > > > Which is sad, because the problem is ours and we're doing a poor job > here. > > Node.js implements a similar pause API and does a better job here. > > I would prefer our APIs remain minimalist in their design. Auto-polling at > the > implementation layer adds value in that it avoids some latency imposed by > the JS > event loop. I don't a similar level of value being added by this buffering > behavior. > > Since pause can't actually stop a pending read from completing or data from > arriving, this buffering only artificially hides an event from the client. > Why > is that better than firing the event and letting the client decide what to > do? > I'm certain that not every client is averse to receiving and processing > some > data after issuing a pause. > > > > This CL is a buffer, some plumbing, and test cases. If it's too big or > > there are ways you can see to make it smaller, I'm happy to break it up > or > > adjust it. > > > We (uProxy) use the socket api quite a bit, and implement quite a bit > > atop: > > a SOCKS5 server, an XHR client, and an XMPP client. We've been hitting > > issues with sockets (basically, pausing and backpressure) for a while, > and > > I'm quite eager to fix them. The workarounds in code are quite painful > > and > > cost us substantial amounts of memory and performance. > > > > > Cheers, > > $ ls > > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering > | > > lally@google.com > > > > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> wrote: > > > > I'm not convinced it needs to be fixed as long as it's understood that > > > there's no guarantee that data won't arrive after pause. I can imagine > > > cases where it's still useful without that guarantee, but your case > > isn't > > > one of them. > > > > > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> wrote: > > > > > >> I'm happy with an N > 1 approach. > > >> > > >> I'm not sure how else to fix the currently-broken setPaused() api > other > > >> than buffering. > > >> > > >> Cheers, > > >> $ ls > > >> > > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | > > Engineering | > > >> lally@google.com > > >> > > >> > > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> > > wrote: > > >> > > >>> I think it would be reasonable (for #5) to have a max pending receive > > >>> option. It would work generally as you describe (don't read until > > event(s) > > >>> have been acked by JS), it would just support N > 1 outstanding > > receive > > >>> events. > > >>> > > >>> Re #4: Doh... right, I see, because we can't just cancel an > > outstanding > > >>> read syscall. In that case I'd like to just continue on a path that > doesn't > > >>> require us to solve that problem, which I think start-paused does. :) > > >>> > > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> > wrote: > > >>> > > >>>> I'm happy with a "start paused" option, and a read() method to > > access a > > >>>> paused stream. I think that's a natural extension of the current > > API. > > >>>> > > >>>> For #4: The problem with just deferring the setPaused() callback is > > >>>> that data may not be coming back anytime soon, so that callback can > > sit > and > > >>>> wait a long time. I'm not sure how to get around that without > buffering. > > >>>> > > >>>> For #5: any thoughts on what kind of throttle specification that'd > > be? > > >>>> A max-outstanding-JS events count, where you can specify a count > > > 1, can > > >>>> keep throughput high. But if we stop talking in terms of > > outstanding JS > > >>>> events, we have to find another way to maintain sensitivity to CPU > pressure. > > >>>> > > >>>> > > >>>> > > >>>> Cheers, > > >>>> $ ls > > >>>> > > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | > > >>>> Engineering | lally@google.com > > >>>> > > >>>> > > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: > > >>>> > > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: > > >>>>> > > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: > > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() implementation. > > >>>>>> setPause() > > >>>>>> > > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the socket, > > >>>>> and then > > >>>>> issues onReceive events back to JS when those Read()s complete. > > >>>>> > > >>>>>> > > > >>>>>> > But, when pausing, any issued Read() continues to live on. > > >>>>>> Currently, that > > >>>>>> > > >>>>> means that another onReceive() is likely to come, even when the > > >>>>> socket's paused. > > >>>>> This change buffers the data that comes back from Read() for > > sending > > >>>>> later, > > >>>>> when the socket is un-paused. > > >>>>> > > >>>>>> > > > >>>>>> > I've written up the issues here: > > >>>>>> > > >>>>> > > >>>>> > > https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ > > >>>>> . > > >>>>> We don't do backpressure here (that's in a follow-up CL), but some > > of > > >>>>> the > > >>>>> testing support needed for that is included here. > > >>>>> > > >>>>>> > > > >>>>>> > > > >>>>>> > Please take a look, and let me know if you have any questions. > > I'm > > >>>>>> happy to > > >>>>>> > > >>>>> talk about this. > > >>>>> > > >>>>>> > > > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in > > >>>>>> reviewing this CL. > > >>>>>> > > >>>>> > > >>>>> This is an unfortunate problem and I agree that we need to fix it, > > but > > >>>>>> this > > >>>>>> > > >>>>> solution seems fairly high in complexity to me. Here is my > > >>>>> counter-proposal for > > >>>>> what we might do: > > >>>>> > > >>>>> 1. Add a read() API that works in a new "manual mode" only > > >>>>>> 2. Add an option on socket creation to create the socket in > > >>>>>> "manual mode" - > > >>>>>> > > >>>>> i.e. no auto-polling for receive > > >>>>> > > >>>>>> 3. Add an API to enable automatic receive polling (i.e. to > leave > > >>>>>> "manual > > >>>>>> > > >>>>> mode"). NO API for switching back to manual mode, because this is > > >>>>> unnecessary > > >>>>> and a source of much complexity here. > > >>>>> > > >>>>> Separately we could also address two (IMHO) lower priority issues: > > >>>>>> > > >>>>> > > >>>>> 4. Defer the setPaused callback until the receive queue is > > flushed, > > >>>>>> just > > >>>>>> > > >>>>> because this feels like the right thing to do. > > >>>>> > > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, to > > avoid > > >>>>>> the > > >>>>>> > > >>>>> over-eager reading problem. > > >>>>> > > >>>>> I think adding 1-3 would be lower complexity than what you have > > here, > > >>>>>> AFAICT > > >>>>>> > > >>>>> it would address all of your needs, and it doesn't actually change > > any > > >>>>> behavior > > >>>>> of existing API uses in the wild (for better or worse). > > >>>>> > > >>>>> #4 is nice to have, #5 seems to me like a more flexible throttling > > >>>>>> solution > > >>>>>> > > >>>>> than strictly tying read call frequency to the JS event loop. > > >>>>> > > >>>>> WDYT? > > >>>>>> > > >>>>> > > >>>>> Or maybe instead of "manual mode" which is a bit confusing, we have > > a > > >>>>> "start > > >>>>> paused" option. Then read() is, as you suggest, a way of reading > > from > > >>>>> the socket > > >>>>> while paused. The main difference is that we can avoid trying to > > >>>>> resolve the > > >>>>> complexity of pausing at a very specific place in the stream, by > > >>>>> making it > > >>>>> possible to start the socket in a paused state. > > >>>>> > > >>>>> The trade-off is that you have to do manual reads until you're > ready > > >>>>> to initiate > > >>>>> TLS and unpause, but that doesn't seem so bad to me. > > >>>>> > > >>>>> > > >>>>> https://codereview.chromium.org/494573002/ > > >>>>> > > >>>> > > >>>> > > >>> > > >> > > > > > > -- > > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/494573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm happy to schedule a VC, but I've read the document, and we've discussed alternative ways of addressing both the secure socket flow and the back-pressure problem, neither of which seem to require another layer of implementation-level buffering to be solved. To summarize the conclusions I drew from that discussion: 1. If creating a socket in a paused state helps make secure socket flow less hacky to implement (seems like it does?), then the details of set-paused behavior seem largely inconsequential to me and its "broken"ness is rather subjective. 2. Back-pressure can be addressed efficiently with a per-socket option to configure an upper bound on the size of the receive-event backlog. My assumptions around #1 may be invalid. Does managing TLS setup with a manual read() API (prior to initiating TLS and unpausing) add more complexity than I'm imagining? On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> wrote: > I believe Lally shared the the proposed design > <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite > a while ago and it was discussed a bunch then. My understanding is that its purpose > is to fix two bad things with the current API: set-paused is broken, > which means that many secure socket flows need to be hacked/don't work, and > there is no back-pressure, which means you have to implement a very > inefficient version in JS. Having back-pressure and secure sockets behave > properly seems good to me, and is certainly what I'd expect of a tcp socket > library. I suspect there is some basic misunderstanding here, maybe a > short VC would help? > > On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: > >> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >> > In avoiding this, I think we'd just be making clients implement the same >> > buffer in their code. >> >> > Which is sad, because the problem is ours and we're doing a poor job >> here. >> > Node.js implements a similar pause API and does a better job here. >> >> I would prefer our APIs remain minimalist in their design. Auto-polling at >> the >> implementation layer adds value in that it avoids some latency imposed by >> the JS >> event loop. I don't a similar level of value being added by this buffering >> behavior. >> >> Since pause can't actually stop a pending read from completing or data >> from >> arriving, this buffering only artificially hides an event from the client. >> Why >> is that better than firing the event and letting the client decide what to >> do? >> I'm certain that not every client is averse to receiving and processing >> some >> data after issuing a pause. >> >> >> > This CL is a buffer, some plumbing, and test cases. If it's too big or >> > there are ways you can see to make it smaller, I'm happy to break it up >> or >> > adjust it. >> >> > We (uProxy) use the socket api quite a bit, and implement quite a bit >> > atop: >> > a SOCKS5 server, an XHR client, and an XMPP client. We've been hitting >> > issues with sockets (basically, pausing and backpressure) for a while, >> and >> > I'm quite eager to fix them. The workarounds in code are quite painful >> > and >> > cost us substantial amounts of memory and performance. >> >> >> >> > Cheers, >> > $ ls >> >> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >> Engineering | >> > lally@google.com >> >> >> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >> wrote: >> >> > > I'm not convinced it needs to be fixed as long as it's understood that >> > > there's no guarantee that data won't arrive after pause. I can imagine >> > > cases where it's still useful without that guarantee, but your case >> > isn't >> > > one of them. >> > > >> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> wrote: >> > > >> > >> I'm happy with an N > 1 approach. >> > >> >> > >> I'm not sure how else to fix the currently-broken setPaused() api >> other >> > >> than buffering. >> > >> >> > >> Cheers, >> > >> $ ls >> > >> >> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >> > Engineering | >> > >> lally@google.com >> > >> >> > >> >> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> >> > wrote: >> > >> >> > >>> I think it would be reasonable (for #5) to have a max pending >> receive >> > >>> option. It would work generally as you describe (don't read until >> > event(s) >> > >>> have been acked by JS), it would just support N > 1 outstanding >> > receive >> > >>> events. >> > >>> >> > >>> Re #4: Doh... right, I see, because we can't just cancel an >> > outstanding >> > >>> read syscall. In that case I'd like to just continue on a path that >> doesn't >> > >>> require us to solve that problem, which I think start-paused does. >> :) >> > >>> >> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> >> wrote: >> > >>> >> > >>>> I'm happy with a "start paused" option, and a read() method to >> > access a >> > >>>> paused stream. I think that's a natural extension of the current >> > API. >> > >>>> >> > >>>> For #4: The problem with just deferring the setPaused() callback is >> > >>>> that data may not be coming back anytime soon, so that callback can >> > sit >> and >> > >>>> wait a long time. I'm not sure how to get around that without >> buffering. >> > >>>> >> > >>>> For #5: any thoughts on what kind of throttle specification that'd >> > be? >> > >>>> A max-outstanding-JS events count, where you can specify a count > >> > 1, can >> > >>>> keep throughput high. But if we stop talking in terms of >> > outstanding JS >> > >>>> events, we have to find another way to maintain sensitivity to CPU >> pressure. >> > >>>> >> > >>>> >> > >>>> >> > >>>> Cheers, >> > >>>> $ ls >> > >>>> >> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >> > >>>> Engineering | lally@google.com >> > >>>> >> > >>>> >> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >> > >>>> >> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >> > >>>>> >> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >> implementation. >> > >>>>>> setPause() >> > >>>>>> >> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the >> socket, >> > >>>>> and then >> > >>>>> issues onReceive events back to JS when those Read()s complete. >> > >>>>> >> > >>>>>> > >> > >>>>>> > But, when pausing, any issued Read() continues to live on. >> > >>>>>> Currently, that >> > >>>>>> >> > >>>>> means that another onReceive() is likely to come, even when the >> > >>>>> socket's paused. >> > >>>>> This change buffers the data that comes back from Read() for >> > sending >> > >>>>> later, >> > >>>>> when the socket is un-paused. >> > >>>>> >> > >>>>>> > >> > >>>>>> > I've written up the issues here: >> > >>>>>> >> > >>>>> >> > >>>>> >> >> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >> > >>>>> . >> > >>>>> We don't do backpressure here (that's in a follow-up CL), but >> some >> > of >> > >>>>> the >> > >>>>> testing support needed for that is included here. >> > >>>>> >> > >>>>>> > >> > >>>>>> > >> > >>>>>> > Please take a look, and let me know if you have any questions. >> > I'm >> > >>>>>> happy to >> > >>>>>> >> > >>>>> talk about this. >> > >>>>> >> > >>>>>> > >> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in >> > >>>>>> reviewing this CL. >> > >>>>>> >> > >>>>> >> > >>>>> This is an unfortunate problem and I agree that we need to fix it, >> > but >> > >>>>>> this >> > >>>>>> >> > >>>>> solution seems fairly high in complexity to me. Here is my >> > >>>>> counter-proposal for >> > >>>>> what we might do: >> > >>>>> >> > >>>>> 1. Add a read() API that works in a new "manual mode" only >> > >>>>>> 2. Add an option on socket creation to create the socket in >> > >>>>>> "manual mode" - >> > >>>>>> >> > >>>>> i.e. no auto-polling for receive >> > >>>>> >> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. to >> leave >> > >>>>>> "manual >> > >>>>>> >> > >>>>> mode"). NO API for switching back to manual mode, because this is >> > >>>>> unnecessary >> > >>>>> and a source of much complexity here. >> > >>>>> >> > >>>>> Separately we could also address two (IMHO) lower priority issues: >> > >>>>>> >> > >>>>> >> > >>>>> 4. Defer the setPaused callback until the receive queue is >> > flushed, >> > >>>>>> just >> > >>>>>> >> > >>>>> because this feels like the right thing to do. >> > >>>>> >> > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, to >> > avoid >> > >>>>>> the >> > >>>>>> >> > >>>>> over-eager reading problem. >> > >>>>> >> > >>>>> I think adding 1-3 would be lower complexity than what you have >> > here, >> > >>>>>> AFAICT >> > >>>>>> >> > >>>>> it would address all of your needs, and it doesn't actually change >> > any >> > >>>>> behavior >> > >>>>> of existing API uses in the wild (for better or worse). >> > >>>>> >> > >>>>> #4 is nice to have, #5 seems to me like a more flexible throttling >> > >>>>>> solution >> > >>>>>> >> > >>>>> than strictly tying read call frequency to the JS event loop. >> > >>>>> >> > >>>>> WDYT? >> > >>>>>> >> > >>>>> >> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, we >> have >> > a >> > >>>>> "start >> > >>>>> paused" option. Then read() is, as you suggest, a way of reading >> > from >> > >>>>> the socket >> > >>>>> while paused. The main difference is that we can avoid trying to >> > >>>>> resolve the >> > >>>>> complexity of pausing at a very specific place in the stream, by >> > >>>>> making it >> > >>>>> possible to start the socket in a paused state. >> > >>>>> >> > >>>>> The trade-off is that you have to do manual reads until you're >> ready >> > >>>>> to initiate >> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >> > >>>>> >> > >>>>> >> > >>>>> https://codereview.chromium.org/494573002/ >> > >>>>> >> > >>>> >> > >>>> >> > >>> >> > >> >> > > >> >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> "Chromium-reviews" group. >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email to chromium-reviews+unsubscribe@chromium.org. >> >> >> >> >> https://codereview.chromium.org/494573002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
#2 is correct. #1 works fine as long as you have a trivial STARTLS protocol. For XMPP, we'd need to speak both read() and onReceive for the XMPP stack. Two different APIs, but you can bridge them with a buffer. We've got some server-specific hacks to work right now. It's a nasty pain point for us. I don't know if node compatibility was ever a goal for this API -- there are a *lot* of handy libraries you can use in a chrome app, but it's tougher with the different pause semantics between the two apis. Anyways, I'm spent. If these aren't compelling, then I'll dump the CL and push the test infrastructure in here to the other backpressure CL (I wrote it for both). Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> wrote: > I'm happy to schedule a VC, but I've read the document, and we've > discussed alternative ways of addressing both the secure socket flow and > the back-pressure problem, neither of which seem to require another layer > of implementation-level buffering to be solved. To summarize the > conclusions I drew from that discussion: > > 1. If creating a socket in a paused state helps make secure socket > flow less hacky to implement (seems like it does?), then the details of > set-paused behavior seem largely inconsequential to me and its "broken"ness > is rather subjective. > 2. Back-pressure can be addressed efficiently with a per-socket option > to configure an upper bound on the size of the receive-event backlog. > > My assumptions around #1 may be invalid. Does managing TLS setup with a > manual read() API (prior to initiating TLS and unpausing) add more > complexity than I'm imagining? > > > On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> wrote: > >> I believe Lally shared the the proposed design >> <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite >> a while ago and it was discussed a bunch then. My understanding is that its purpose >> is to fix two bad things with the current API: set-paused is broken, >> which means that many secure socket flows need to be hacked/don't work, and >> there is no back-pressure, which means you have to implement a very >> inefficient version in JS. Having back-pressure and secure sockets behave >> properly seems good to me, and is certainly what I'd expect of a tcp socket >> library. I suspect there is some basic misunderstanding here, maybe a >> short VC would help? >> >> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: >> >>> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >>> > In avoiding this, I think we'd just be making clients implement the >>> same >>> > buffer in their code. >>> >>> > Which is sad, because the problem is ours and we're doing a poor job >>> here. >>> > Node.js implements a similar pause API and does a better job here. >>> >>> I would prefer our APIs remain minimalist in their design. Auto-polling >>> at >>> the >>> implementation layer adds value in that it avoids some latency imposed by >>> the JS >>> event loop. I don't a similar level of value being added by this >>> buffering >>> behavior. >>> >>> Since pause can't actually stop a pending read from completing or data >>> from >>> arriving, this buffering only artificially hides an event from the >>> client. >>> Why >>> is that better than firing the event and letting the client decide what >>> to >>> do? >>> I'm certain that not every client is averse to receiving and processing >>> some >>> data after issuing a pause. >>> >>> >>> > This CL is a buffer, some plumbing, and test cases. If it's too big >>> or >>> > there are ways you can see to make it smaller, I'm happy to break it >>> up or >>> > adjust it. >>> >>> > We (uProxy) use the socket api quite a bit, and implement quite a bit >>> > atop: >>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been hitting >>> > issues with sockets (basically, pausing and backpressure) for a while, >>> and >>> > I'm quite eager to fix them. The workarounds in code are quite painful >>> > and >>> > cost us substantial amounts of memory and performance. >>> >>> >>> >>> > Cheers, >>> > $ ls >>> >>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>> Engineering | >>> > lally@google.com >>> >>> >>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >>> wrote: >>> >>> > > I'm not convinced it needs to be fixed as long as it's understood >>> that >>> > > there's no guarantee that data won't arrive after pause. I can >>> imagine >>> > > cases where it's still useful without that guarantee, but your case >>> > isn't >>> > > one of them. >>> > > >>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> >>> wrote: >>> > > >>> > >> I'm happy with an N > 1 approach. >>> > >> >>> > >> I'm not sure how else to fix the currently-broken setPaused() api >>> other >>> > >> than buffering. >>> > >> >>> > >> Cheers, >>> > >> $ ls >>> > >> >>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>> > Engineering | >>> > >> lally@google.com >>> > >> >>> > >> >>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> >>> > wrote: >>> > >> >>> > >>> I think it would be reasonable (for #5) to have a max pending >>> receive >>> > >>> option. It would work generally as you describe (don't read until >>> > event(s) >>> > >>> have been acked by JS), it would just support N > 1 outstanding >>> > receive >>> > >>> events. >>> > >>> >>> > >>> Re #4: Doh... right, I see, because we can't just cancel an >>> > outstanding >>> > >>> read syscall. In that case I'd like to just continue on a path that >>> doesn't >>> > >>> require us to solve that problem, which I think start-paused does. >>> :) >>> > >>> >>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> >>> wrote: >>> > >>> >>> > >>>> I'm happy with a "start paused" option, and a read() method to >>> > access a >>> > >>>> paused stream. I think that's a natural extension of the current >>> > API. >>> > >>>> >>> > >>>> For #4: The problem with just deferring the setPaused() callback >>> is >>> > >>>> that data may not be coming back anytime soon, so that callback >>> can >>> > sit >>> and >>> > >>>> wait a long time. I'm not sure how to get around that without >>> buffering. >>> > >>>> >>> > >>>> For #5: any thoughts on what kind of throttle specification that'd >>> > be? >>> > >>>> A max-outstanding-JS events count, where you can specify a count > >>> > 1, can >>> > >>>> keep throughput high. But if we stop talking in terms of >>> > outstanding JS >>> > >>>> events, we have to find another way to maintain sensitivity to CPU >>> pressure. >>> > >>>> >>> > >>>> >>> > >>>> >>> > >>>> Cheers, >>> > >>>> $ ls >>> > >>>> >>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>> > >>>> Engineering | lally@google.com >>> > >>>> >>> > >>>> >>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >>> > >>>> >>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>> > >>>>> >>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >>> implementation. >>> > >>>>>> setPause() >>> > >>>>>> >>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the >>> socket, >>> > >>>>> and then >>> > >>>>> issues onReceive events back to JS when those Read()s complete. >>> > >>>>> >>> > >>>>>> > >>> > >>>>>> > But, when pausing, any issued Read() continues to live on. >>> > >>>>>> Currently, that >>> > >>>>>> >>> > >>>>> means that another onReceive() is likely to come, even when the >>> > >>>>> socket's paused. >>> > >>>>> This change buffers the data that comes back from Read() for >>> > sending >>> > >>>>> later, >>> > >>>>> when the socket is un-paused. >>> > >>>>> >>> > >>>>>> > >>> > >>>>>> > I've written up the issues here: >>> > >>>>>> >>> > >>>>> >>> > >>>>> >>> >>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>> > >>>>> . >>> > >>>>> We don't do backpressure here (that's in a follow-up CL), but >>> some >>> > of >>> > >>>>> the >>> > >>>>> testing support needed for that is included here. >>> > >>>>> >>> > >>>>>> > >>> > >>>>>> > >>> > >>>>>> > Please take a look, and let me know if you have any questions. >>> > I'm >>> > >>>>>> happy to >>> > >>>>>> >>> > >>>>> talk about this. >>> > >>>>> >>> > >>>>>> > >>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in >>> > >>>>>> reviewing this CL. >>> > >>>>>> >>> > >>>>> >>> > >>>>> This is an unfortunate problem and I agree that we need to fix >>> it, >>> > but >>> > >>>>>> this >>> > >>>>>> >>> > >>>>> solution seems fairly high in complexity to me. Here is my >>> > >>>>> counter-proposal for >>> > >>>>> what we might do: >>> > >>>>> >>> > >>>>> 1. Add a read() API that works in a new "manual mode" only >>> > >>>>>> 2. Add an option on socket creation to create the socket in >>> > >>>>>> "manual mode" - >>> > >>>>>> >>> > >>>>> i.e. no auto-polling for receive >>> > >>>>> >>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. to >>> leave >>> > >>>>>> "manual >>> > >>>>>> >>> > >>>>> mode"). NO API for switching back to manual mode, because this is >>> > >>>>> unnecessary >>> > >>>>> and a source of much complexity here. >>> > >>>>> >>> > >>>>> Separately we could also address two (IMHO) lower priority >>> issues: >>> > >>>>>> >>> > >>>>> >>> > >>>>> 4. Defer the setPaused callback until the receive queue is >>> > flushed, >>> > >>>>>> just >>> > >>>>>> >>> > >>>>> because this feels like the right thing to do. >>> > >>>>> >>> > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, to >>> > avoid >>> > >>>>>> the >>> > >>>>>> >>> > >>>>> over-eager reading problem. >>> > >>>>> >>> > >>>>> I think adding 1-3 would be lower complexity than what you have >>> > here, >>> > >>>>>> AFAICT >>> > >>>>>> >>> > >>>>> it would address all of your needs, and it doesn't actually >>> change >>> > any >>> > >>>>> behavior >>> > >>>>> of existing API uses in the wild (for better or worse). >>> > >>>>> >>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible >>> throttling >>> > >>>>>> solution >>> > >>>>>> >>> > >>>>> than strictly tying read call frequency to the JS event loop. >>> > >>>>> >>> > >>>>> WDYT? >>> > >>>>>> >>> > >>>>> >>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, we >>> have >>> > a >>> > >>>>> "start >>> > >>>>> paused" option. Then read() is, as you suggest, a way of reading >>> > from >>> > >>>>> the socket >>> > >>>>> while paused. The main difference is that we can avoid trying to >>> > >>>>> resolve the >>> > >>>>> complexity of pausing at a very specific place in the stream, by >>> > >>>>> making it >>> > >>>>> possible to start the socket in a paused state. >>> > >>>>> >>> > >>>>> The trade-off is that you have to do manual reads until you're >>> ready >>> > >>>>> to initiate >>> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >>> > >>>>> >>> > >>>>> >>> > >>>>> https://codereview.chromium.org/494573002/ >>> > >>>>> >>> > >>>> >>> > >>>> >>> > >>> >>> > >> >>> > > >>> >>> > -- >>> > You received this message because you are subscribed to the Google >>> Groups >>> "Chromium-reviews" group. >>> > To unsubscribe from this group and stop receiving emails from it, send >>> an >>> email to chromium-reviews+unsubscribe@chromium.org. >>> >>> >>> >>> >>> https://codereview.chromium.org/494573002/ >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Dec 10, 2015 at 10:52 AM, Lally Singh <lally@google.com> wrote: > #2 is correct. > > #1 works fine as long as you have a trivial STARTLS protocol. For XMPP, > we'd need to speak both read() and onReceive for the XMPP stack. Two > different APIs, but you can bridge them with a buffer. We've got some > server-specific hacks to work right now. It's a nasty pain point for us. > > I don't know if node compatibility was ever a goal for this API -- there > are a *lot* of handy libraries you can use in a chrome app, but it's > tougher with the different pause semantics between the two apis. > > Anyways, I'm spent. If these aren't compelling, then I'll dump the CL and > push the test infrastructure in here to the other backpressure CL (I wrote > it for both). > Sorry this is being drawn out so much. It's helpful to understand the requirements here in more detail. I don't mean to be obtuse, I just want to make sure we have the right API. If start-paused is still painful, it's not a good option. Server-specific hacks sound painful. > > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> wrote: > >> I'm happy to schedule a VC, but I've read the document, and we've >> discussed alternative ways of addressing both the secure socket flow and >> the back-pressure problem, neither of which seem to require another layer >> of implementation-level buffering to be solved. To summarize the >> conclusions I drew from that discussion: >> >> 1. If creating a socket in a paused state helps make secure socket >> flow less hacky to implement (seems like it does?), then the details of >> set-paused behavior seem largely inconsequential to me and its "broken"ness >> is rather subjective. >> 2. Back-pressure can be addressed efficiently with a per-socket >> option to configure an upper bound on the size of the receive-event backlog. >> >> My assumptions around #1 may be invalid. Does managing TLS setup with a >> manual read() API (prior to initiating TLS and unpausing) add more >> complexity than I'm imagining? >> >> >> On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> wrote: >> >>> I believe Lally shared the the proposed design >>> <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite >>> a while ago and it was discussed a bunch then. My understanding is that its purpose >>> is to fix two bad things with the current API: set-paused is broken, >>> which means that many secure socket flows need to be hacked/don't work, and >>> there is no back-pressure, which means you have to implement a very >>> inefficient version in JS. Having back-pressure and secure sockets behave >>> properly seems good to me, and is certainly what I'd expect of a tcp socket >>> library. I suspect there is some basic misunderstanding here, maybe a >>> short VC would help? >>> >>> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: >>> >>>> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >>>> > In avoiding this, I think we'd just be making clients implement the >>>> same >>>> > buffer in their code. >>>> >>>> > Which is sad, because the problem is ours and we're doing a poor job >>>> here. >>>> > Node.js implements a similar pause API and does a better job here. >>>> >>>> I would prefer our APIs remain minimalist in their design. Auto-polling >>>> at >>>> the >>>> implementation layer adds value in that it avoids some latency imposed >>>> by >>>> the JS >>>> event loop. I don't a similar level of value being added by this >>>> buffering >>>> behavior. >>>> >>>> Since pause can't actually stop a pending read from completing or data >>>> from >>>> arriving, this buffering only artificially hides an event from the >>>> client. >>>> Why >>>> is that better than firing the event and letting the client decide what >>>> to >>>> do? >>>> I'm certain that not every client is averse to receiving and processing >>>> some >>>> data after issuing a pause. >>>> >>>> >>>> > This CL is a buffer, some plumbing, and test cases. If it's too big >>>> or >>>> > there are ways you can see to make it smaller, I'm happy to break it >>>> up or >>>> > adjust it. >>>> >>>> > We (uProxy) use the socket api quite a bit, and implement quite a bit >>>> > atop: >>>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been >>>> hitting >>>> > issues with sockets (basically, pausing and backpressure) for a >>>> while, and >>>> > I'm quite eager to fix them. The workarounds in code are quite >>>> painful >>>> > and >>>> > cost us substantial amounts of memory and performance. >>>> >>>> >>>> >>>> > Cheers, >>>> > $ ls >>>> >>>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>> Engineering | >>>> > lally@google.com >>>> >>>> >>>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >>>> wrote: >>>> >>>> > > I'm not convinced it needs to be fixed as long as it's understood >>>> that >>>> > > there's no guarantee that data won't arrive after pause. I can >>>> imagine >>>> > > cases where it's still useful without that guarantee, but your case >>>> > isn't >>>> > > one of them. >>>> > > >>>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> >>>> wrote: >>>> > > >>>> > >> I'm happy with an N > 1 approach. >>>> > >> >>>> > >> I'm not sure how else to fix the currently-broken setPaused() api >>>> other >>>> > >> than buffering. >>>> > >> >>>> > >> Cheers, >>>> > >> $ ls >>>> > >> >>>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>> > Engineering | >>>> > >> lally@google.com >>>> > >> >>>> > >> >>>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> >>>> > wrote: >>>> > >> >>>> > >>> I think it would be reasonable (for #5) to have a max pending >>>> receive >>>> > >>> option. It would work generally as you describe (don't read until >>>> > event(s) >>>> > >>> have been acked by JS), it would just support N > 1 outstanding >>>> > receive >>>> > >>> events. >>>> > >>> >>>> > >>> Re #4: Doh... right, I see, because we can't just cancel an >>>> > outstanding >>>> > >>> read syscall. In that case I'd like to just continue on a path >>>> that >>>> doesn't >>>> > >>> require us to solve that problem, which I think start-paused >>>> does. :) >>>> > >>> >>>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> >>>> wrote: >>>> > >>> >>>> > >>>> I'm happy with a "start paused" option, and a read() method to >>>> > access a >>>> > >>>> paused stream. I think that's a natural extension of the current >>>> > API. >>>> > >>>> >>>> > >>>> For #4: The problem with just deferring the setPaused() callback >>>> is >>>> > >>>> that data may not be coming back anytime soon, so that callback >>>> can >>>> > sit >>>> and >>>> > >>>> wait a long time. I'm not sure how to get around that without >>>> buffering. >>>> > >>>> >>>> > >>>> For #5: any thoughts on what kind of throttle specification >>>> that'd >>>> > be? >>>> > >>>> A max-outstanding-JS events count, where you can specify a count >>>> > >>>> > 1, can >>>> > >>>> keep throughput high. But if we stop talking in terms of >>>> > outstanding JS >>>> > >>>> events, we have to find another way to maintain sensitivity to >>>> CPU >>>> pressure. >>>> > >>>> >>>> > >>>> >>>> > >>>> >>>> > >>>> Cheers, >>>> > >>>> $ ls >>>> > >>>> >>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>> > >>>> Engineering | lally@google.com >>>> > >>>> >>>> > >>>> >>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >>>> > >>>> >>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>> > >>>>> >>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >>>> implementation. >>>> > >>>>>> setPause() >>>> > >>>>>> >>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the >>>> socket, >>>> > >>>>> and then >>>> > >>>>> issues onReceive events back to JS when those Read()s complete. >>>> > >>>>> >>>> > >>>>>> > >>>> > >>>>>> > But, when pausing, any issued Read() continues to live on. >>>> > >>>>>> Currently, that >>>> > >>>>>> >>>> > >>>>> means that another onReceive() is likely to come, even when the >>>> > >>>>> socket's paused. >>>> > >>>>> This change buffers the data that comes back from Read() for >>>> > sending >>>> > >>>>> later, >>>> > >>>>> when the socket is un-paused. >>>> > >>>>> >>>> > >>>>>> > >>>> > >>>>>> > I've written up the issues here: >>>> > >>>>>> >>>> > >>>>> >>>> > >>>>> >>>> >>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>> > >>>>> . >>>> > >>>>> We don't do backpressure here (that's in a follow-up CL), but >>>> some >>>> > of >>>> > >>>>> the >>>> > >>>>> testing support needed for that is included here. >>>> > >>>>> >>>> > >>>>>> > >>>> > >>>>>> > >>>> > >>>>>> > Please take a look, and let me know if you have any >>>> questions. >>>> > I'm >>>> > >>>>>> happy to >>>> > >>>>>> >>>> > >>>>> talk about this. >>>> > >>>>> >>>> > >>>>>> > >>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in >>>> > >>>>>> reviewing this CL. >>>> > >>>>>> >>>> > >>>>> >>>> > >>>>> This is an unfortunate problem and I agree that we need to fix >>>> it, >>>> > but >>>> > >>>>>> this >>>> > >>>>>> >>>> > >>>>> solution seems fairly high in complexity to me. Here is my >>>> > >>>>> counter-proposal for >>>> > >>>>> what we might do: >>>> > >>>>> >>>> > >>>>> 1. Add a read() API that works in a new "manual mode" only >>>> > >>>>>> 2. Add an option on socket creation to create the socket in >>>> > >>>>>> "manual mode" - >>>> > >>>>>> >>>> > >>>>> i.e. no auto-polling for receive >>>> > >>>>> >>>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. to >>>> leave >>>> > >>>>>> "manual >>>> > >>>>>> >>>> > >>>>> mode"). NO API for switching back to manual mode, because this >>>> is >>>> > >>>>> unnecessary >>>> > >>>>> and a source of much complexity here. >>>> > >>>>> >>>> > >>>>> Separately we could also address two (IMHO) lower priority >>>> issues: >>>> > >>>>>> >>>> > >>>>> >>>> > >>>>> 4. Defer the setPaused callback until the receive queue is >>>> > flushed, >>>> > >>>>>> just >>>> > >>>>>> >>>> > >>>>> because this feels like the right thing to do. >>>> > >>>>> >>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, to >>>> > avoid >>>> > >>>>>> the >>>> > >>>>>> >>>> > >>>>> over-eager reading problem. >>>> > >>>>> >>>> > >>>>> I think adding 1-3 would be lower complexity than what you have >>>> > here, >>>> > >>>>>> AFAICT >>>> > >>>>>> >>>> > >>>>> it would address all of your needs, and it doesn't actually >>>> change >>>> > any >>>> > >>>>> behavior >>>> > >>>>> of existing API uses in the wild (for better or worse). >>>> > >>>>> >>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible >>>> throttling >>>> > >>>>>> solution >>>> > >>>>>> >>>> > >>>>> than strictly tying read call frequency to the JS event loop. >>>> > >>>>> >>>> > >>>>> WDYT? >>>> > >>>>>> >>>> > >>>>> >>>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, we >>>> have >>>> > a >>>> > >>>>> "start >>>> > >>>>> paused" option. Then read() is, as you suggest, a way of reading >>>> > from >>>> > >>>>> the socket >>>> > >>>>> while paused. The main difference is that we can avoid trying to >>>> > >>>>> resolve the >>>> > >>>>> complexity of pausing at a very specific place in the stream, by >>>> > >>>>> making it >>>> > >>>>> possible to start the socket in a paused state. >>>> > >>>>> >>>> > >>>>> The trade-off is that you have to do manual reads until you're >>>> ready >>>> > >>>>> to initiate >>>> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >>>> > >>>>> >>>> > >>>>> >>>> > >>>>> https://codereview.chromium.org/494573002/ >>>> > >>>>> >>>> > >>>> >>>> > >>>> >>>> > >>> >>>> > >> >>>> > > >>>> >>>> > -- >>>> > You received this message because you are subscribed to the Google >>>> Groups >>>> "Chromium-reviews" group. >>>> > To unsubscribe from this group and stop receiving emails from it, >>>> send an >>>> email to chromium-reviews+unsubscribe@chromium.org. >>>> >>>> >>>> >>>> >>>> https://codereview.chromium.org/494573002/ >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I asked around the team for where we were having and seeing pain points: * We use setpaused() for flow control. Even with backpressure, we still have to stop the incoming flow sometimes to let our internal queues drain out. The additional slop of an additional onReceive after we've already determined that we want to stop the influx reduces our control over the flow rates. We have a lot of simultaneous connections, so this adds up. * An external group doing IMAP: https://github.com/MobileChromeApps/mobile-chrome-apps/issues/269#issuecommen... - their workaround counts incoming onReceive packets and times a transmit right after a pause to get the sequencing right. But there are a bunch of API users on that thread, it's tough to tell what'll work for all of them. * From a team member here: > Yeah, this was a pain for me - it took a while to figure out what I was > doing wrong and realize it was an issue with the API rather than my code. > Then to fix it, I had to write a new "prepareSecure" method for both > freedom-for-firefox (a no-op) and freedom-for-chrome (calls .pause), plus > the common API definition in freedomjs and call it 1 read prior to wanting > to secure the socket. > > In the case of XMPP starttls flow we were able to work around this because > XMPP works like this: > > 1. client sends init message to server in cleartext > 2. server sends response in cleartext > 3. client sends some more data in cleartext > 4. server replies with starttls command in cleartext > 5. client upgrades the socket to TCP > > Because this flow is predictable we can call prepareSecure (pause) before > step 3 as we know there will be one more read coming up, and then at step 5 > we can actually call .secure. But there may be other protocols where this > doesn't work - the user might call .pause yet if the server doesn't reply > with anything then .pause will never fulfill and they can never call .secure > [ Note that this is all talking XMPP, which is nontrival to implement on both read() and onReceive ] And we've got 19 stars on one of the bugs on this (crbug.com/403076) and 23 on a duplicate (crbug.com/467677) -- but I couldn't tell you about how many of those are overlaps. In contrast, combining this setPause() change with the backpressure CL, and setting N=1 (although I'll amend that CL to allow N to be adjustable) gives you completely precise socket reads. You can pause a socket during an onReceive event handler and it'll take effect immediately. That's very useful if you want to read the next K bytes to determine what to do next , set your onReceive event handlers to different functions, depending on what value you got for the K bytes, and then unpause the socket. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Thu, Dec 10, 2015 at 2:00 PM, Ken Rockot <rockot@chromium.org> wrote: > > > On Thu, Dec 10, 2015 at 10:52 AM, Lally Singh <lally@google.com> wrote: > >> #2 is correct. >> >> #1 works fine as long as you have a trivial STARTLS protocol. For XMPP, >> we'd need to speak both read() and onReceive for the XMPP stack. Two >> different APIs, but you can bridge them with a buffer. We've got some >> server-specific hacks to work right now. It's a nasty pain point for us. >> >> I don't know if node compatibility was ever a goal for this API -- there >> are a *lot* of handy libraries you can use in a chrome app, but it's >> tougher with the different pause semantics between the two apis. >> >> Anyways, I'm spent. If these aren't compelling, then I'll dump the CL >> and push the test infrastructure in here to the other backpressure CL (I >> wrote it for both). >> > > Sorry this is being drawn out so much. It's helpful to understand the > requirements here in more detail. I don't mean to be obtuse, I just want to > make sure we have the right API. If start-paused is still painful, it's not > a good option. Server-specific hacks sound painful. > > >> >> >> >> Cheers, >> $ ls >> >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | >> lally@google.com >> >> >> On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> wrote: >> >>> I'm happy to schedule a VC, but I've read the document, and we've >>> discussed alternative ways of addressing both the secure socket flow and >>> the back-pressure problem, neither of which seem to require another layer >>> of implementation-level buffering to be solved. To summarize the >>> conclusions I drew from that discussion: >>> >>> 1. If creating a socket in a paused state helps make secure socket >>> flow less hacky to implement (seems like it does?), then the details of >>> set-paused behavior seem largely inconsequential to me and its "broken"ness >>> is rather subjective. >>> 2. Back-pressure can be addressed efficiently with a per-socket >>> option to configure an upper bound on the size of the receive-event backlog. >>> >>> My assumptions around #1 may be invalid. Does managing TLS setup with a >>> manual read() API (prior to initiating TLS and unpausing) add more >>> complexity than I'm imagining? >>> >>> >>> On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> wrote: >>> >>>> I believe Lally shared the the proposed design >>>> <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite >>>> a while ago and it was discussed a bunch then. My understanding is that its purpose >>>> is to fix two bad things with the current API: set-paused is broken, >>>> which means that many secure socket flows need to be hacked/don't work, and >>>> there is no back-pressure, which means you have to implement a very >>>> inefficient version in JS. Having back-pressure and secure sockets behave >>>> properly seems good to me, and is certainly what I'd expect of a tcp socket >>>> library. I suspect there is some basic misunderstanding here, maybe a >>>> short VC would help? >>>> >>>> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: >>>> >>>>> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >>>>> > In avoiding this, I think we'd just be making clients implement the >>>>> same >>>>> > buffer in their code. >>>>> >>>>> > Which is sad, because the problem is ours and we're doing a poor job >>>>> here. >>>>> > Node.js implements a similar pause API and does a better job here. >>>>> >>>>> I would prefer our APIs remain minimalist in their design. >>>>> Auto-polling at >>>>> the >>>>> implementation layer adds value in that it avoids some latency imposed >>>>> by >>>>> the JS >>>>> event loop. I don't a similar level of value being added by this >>>>> buffering >>>>> behavior. >>>>> >>>>> Since pause can't actually stop a pending read from completing or data >>>>> from >>>>> arriving, this buffering only artificially hides an event from the >>>>> client. >>>>> Why >>>>> is that better than firing the event and letting the client decide >>>>> what to >>>>> do? >>>>> I'm certain that not every client is averse to receiving and >>>>> processing some >>>>> data after issuing a pause. >>>>> >>>>> >>>>> > This CL is a buffer, some plumbing, and test cases. If it's too >>>>> big or >>>>> > there are ways you can see to make it smaller, I'm happy to break it >>>>> up or >>>>> > adjust it. >>>>> >>>>> > We (uProxy) use the socket api quite a bit, and implement quite a bit >>>>> > atop: >>>>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been >>>>> hitting >>>>> > issues with sockets (basically, pausing and backpressure) for a >>>>> while, and >>>>> > I'm quite eager to fix them. The workarounds in code are quite >>>>> painful >>>>> > and >>>>> > cost us substantial amounts of memory and performance. >>>>> >>>>> >>>>> >>>>> > Cheers, >>>>> > $ ls >>>>> >>>>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>> Engineering | >>>>> > lally@google.com >>>>> >>>>> >>>>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >>>>> wrote: >>>>> >>>>> > > I'm not convinced it needs to be fixed as long as it's understood >>>>> that >>>>> > > there's no guarantee that data won't arrive after pause. I can >>>>> imagine >>>>> > > cases where it's still useful without that guarantee, but your case >>>>> > isn't >>>>> > > one of them. >>>>> > > >>>>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> >>>>> wrote: >>>>> > > >>>>> > >> I'm happy with an N > 1 approach. >>>>> > >> >>>>> > >> I'm not sure how else to fix the currently-broken setPaused() api >>>>> other >>>>> > >> than buffering. >>>>> > >> >>>>> > >> Cheers, >>>>> > >> $ ls >>>>> > >> >>>>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>> > Engineering | >>>>> > >> lally@google.com >>>>> > >> >>>>> > >> >>>>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org> >>>>> > wrote: >>>>> > >> >>>>> > >>> I think it would be reasonable (for #5) to have a max pending >>>>> receive >>>>> > >>> option. It would work generally as you describe (don't read until >>>>> > event(s) >>>>> > >>> have been acked by JS), it would just support N > 1 outstanding >>>>> > receive >>>>> > >>> events. >>>>> > >>> >>>>> > >>> Re #4: Doh... right, I see, because we can't just cancel an >>>>> > outstanding >>>>> > >>> read syscall. In that case I'd like to just continue on a path >>>>> that >>>>> doesn't >>>>> > >>> require us to solve that problem, which I think start-paused >>>>> does. :) >>>>> > >>> >>>>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> >>>>> wrote: >>>>> > >>> >>>>> > >>>> I'm happy with a "start paused" option, and a read() method to >>>>> > access a >>>>> > >>>> paused stream. I think that's a natural extension of the >>>>> current >>>>> > API. >>>>> > >>>> >>>>> > >>>> For #4: The problem with just deferring the setPaused() >>>>> callback is >>>>> > >>>> that data may not be coming back anytime soon, so that callback >>>>> can >>>>> > sit >>>>> and >>>>> > >>>> wait a long time. I'm not sure how to get around that without >>>>> buffering. >>>>> > >>>> >>>>> > >>>> For #5: any thoughts on what kind of throttle specification >>>>> that'd >>>>> > be? >>>>> > >>>> A max-outstanding-JS events count, where you can specify a >>>>> count > >>>>> > 1, can >>>>> > >>>> keep throughput high. But if we stop talking in terms of >>>>> > outstanding JS >>>>> > >>>> events, we have to find another way to maintain sensitivity to >>>>> CPU >>>>> pressure. >>>>> > >>>> >>>>> > >>>> >>>>> > >>>> >>>>> > >>>> Cheers, >>>>> > >>>> $ ls >>>>> > >>>> >>>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>> > >>>> Engineering | lally@google.com >>>>> > >>>> >>>>> > >>>> >>>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >>>>> > >>>> >>>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>>> > >>>>> >>>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >>>>> implementation. >>>>> > >>>>>> setPause() >>>>> > >>>>>> >>>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the >>>>> socket, >>>>> > >>>>> and then >>>>> > >>>>> issues onReceive events back to JS when those Read()s complete. >>>>> > >>>>> >>>>> > >>>>>> > >>>>> > >>>>>> > But, when pausing, any issued Read() continues to live on. >>>>> > >>>>>> Currently, that >>>>> > >>>>>> >>>>> > >>>>> means that another onReceive() is likely to come, even when the >>>>> > >>>>> socket's paused. >>>>> > >>>>> This change buffers the data that comes back from Read() for >>>>> > sending >>>>> > >>>>> later, >>>>> > >>>>> when the socket is un-paused. >>>>> > >>>>> >>>>> > >>>>>> > >>>>> > >>>>>> > I've written up the issues here: >>>>> > >>>>>> >>>>> > >>>>> >>>>> > >>>>> >>>>> >>>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>>> > >>>>> . >>>>> > >>>>> We don't do backpressure here (that's in a follow-up CL), but >>>>> some >>>>> > of >>>>> > >>>>> the >>>>> > >>>>> testing support needed for that is included here. >>>>> > >>>>> >>>>> > >>>>>> > >>>>> > >>>>>> > >>>>> > >>>>>> > Please take a look, and let me know if you have any >>>>> questions. >>>>> > I'm >>>>> > >>>>>> happy to >>>>> > >>>>>> >>>>> > >>>>> talk about this. >>>>> > >>>>> >>>>> > >>>>>> > >>>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in >>>>> > >>>>>> reviewing this CL. >>>>> > >>>>>> >>>>> > >>>>> >>>>> > >>>>> This is an unfortunate problem and I agree that we need to fix >>>>> it, >>>>> > but >>>>> > >>>>>> this >>>>> > >>>>>> >>>>> > >>>>> solution seems fairly high in complexity to me. Here is my >>>>> > >>>>> counter-proposal for >>>>> > >>>>> what we might do: >>>>> > >>>>> >>>>> > >>>>> 1. Add a read() API that works in a new "manual mode" only >>>>> > >>>>>> 2. Add an option on socket creation to create the socket in >>>>> > >>>>>> "manual mode" - >>>>> > >>>>>> >>>>> > >>>>> i.e. no auto-polling for receive >>>>> > >>>>> >>>>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. to >>>>> leave >>>>> > >>>>>> "manual >>>>> > >>>>>> >>>>> > >>>>> mode"). NO API for switching back to manual mode, because this >>>>> is >>>>> > >>>>> unnecessary >>>>> > >>>>> and a source of much complexity here. >>>>> > >>>>> >>>>> > >>>>> Separately we could also address two (IMHO) lower priority >>>>> issues: >>>>> > >>>>>> >>>>> > >>>>> >>>>> > >>>>> 4. Defer the setPaused callback until the receive queue is >>>>> > flushed, >>>>> > >>>>>> just >>>>> > >>>>>> >>>>> > >>>>> because this feels like the right thing to do. >>>>> > >>>>> >>>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, >>>>> to >>>>> > avoid >>>>> > >>>>>> the >>>>> > >>>>>> >>>>> > >>>>> over-eager reading problem. >>>>> > >>>>> >>>>> > >>>>> I think adding 1-3 would be lower complexity than what you have >>>>> > here, >>>>> > >>>>>> AFAICT >>>>> > >>>>>> >>>>> > >>>>> it would address all of your needs, and it doesn't actually >>>>> change >>>>> > any >>>>> > >>>>> behavior >>>>> > >>>>> of existing API uses in the wild (for better or worse). >>>>> > >>>>> >>>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible >>>>> throttling >>>>> > >>>>>> solution >>>>> > >>>>>> >>>>> > >>>>> than strictly tying read call frequency to the JS event loop. >>>>> > >>>>> >>>>> > >>>>> WDYT? >>>>> > >>>>>> >>>>> > >>>>> >>>>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, we >>>>> have >>>>> > a >>>>> > >>>>> "start >>>>> > >>>>> paused" option. Then read() is, as you suggest, a way of >>>>> reading >>>>> > from >>>>> > >>>>> the socket >>>>> > >>>>> while paused. The main difference is that we can avoid trying >>>>> to >>>>> > >>>>> resolve the >>>>> > >>>>> complexity of pausing at a very specific place in the stream, >>>>> by >>>>> > >>>>> making it >>>>> > >>>>> possible to start the socket in a paused state. >>>>> > >>>>> >>>>> > >>>>> The trade-off is that you have to do manual reads until you're >>>>> ready >>>>> > >>>>> to initiate >>>>> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >>>>> > >>>>> >>>>> > >>>>> >>>>> > >>>>> https://codereview.chromium.org/494573002/ >>>>> > >>>>> >>>>> > >>>> >>>>> > >>>> >>>>> > >>> >>>>> > >> >>>>> > > >>>>> >>>>> > -- >>>>> > You received this message because you are subscribed to the Google >>>>> Groups >>>>> "Chromium-reviews" group. >>>>> > To unsubscribe from this group and stop receiving emails from it, >>>>> send an >>>>> email to chromium-reviews+unsubscribe@chromium.org. >>>>> >>>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/494573002/ >>>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Reading a set of bytes can be important for STARTTLS flows, especially because I don't think, in general, you get a guarantee about what each packet contains. Without precise control of how much gets read, you can't be sure that you pause/read the right amount before encryption starts. On Mon, 14 Dec 2015 at 11:46 Lally Singh <lally@google.com> wrote: > I asked around the team for where we were having and seeing pain points: > > * We use setpaused() for flow control. Even with backpressure, we still > have to stop the incoming flow sometimes to let our internal queues drain > out. The additional slop of an additional onReceive after we've already > determined that we want to stop the influx reduces our control over the > flow rates. We have a lot of simultaneous connections, so this adds up. > > * An external group doing IMAP: > https://github.com/MobileChromeApps/mobile-chrome-apps/issues/269#issuecommen... - > their workaround counts incoming onReceive packets and times a transmit > right after a pause to get the sequencing right. But there are a bunch of > API users on that thread, it's tough to tell what'll work for all of them. > > * From a team member here: > >> Yeah, this was a pain for me - it took a while to figure out what I was >> doing wrong and realize it was an issue with the API rather than my code. >> Then to fix it, I had to write a new "prepareSecure" method for both >> freedom-for-firefox (a no-op) and freedom-for-chrome (calls .pause), plus >> the common API definition in freedomjs and call it 1 read prior to wanting >> to secure the socket. >> >> In the case of XMPP starttls flow we were able to work around this >> because XMPP works like this: >> >> 1. client sends init message to server in cleartext >> 2. server sends response in cleartext >> 3. client sends some more data in cleartext >> 4. server replies with starttls command in cleartext >> 5. client upgrades the socket to TCP >> >> Because this flow is predictable we can call prepareSecure (pause) before >> step 3 as we know there will be one more read coming up, and then at step 5 >> we can actually call .secure. But there may be other protocols where this >> doesn't work - the user might call .pause yet if the server doesn't reply >> with anything then .pause will never fulfill and they can never call .secure >> > [ Note that this is all talking XMPP, which is nontrival to implement on > both read() and onReceive ] > > And we've got 19 stars on one of the bugs on this (crbug.com/403076) and > 23 on a duplicate (crbug.com/467677) -- but I couldn't tell you about how > many of those are overlaps. > > In contrast, combining this setPause() change with the backpressure CL, > and setting N=1 (although I'll amend that CL to allow N to be adjustable) > gives you completely precise socket reads. You can pause a socket during > an onReceive event handler and it'll take effect immediately. That's very > useful if you want to read the next K bytes to determine what to do next , > set your onReceive event handlers to different functions, depending on what > value you got for the K bytes, and then unpause the socket. > > > > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Thu, Dec 10, 2015 at 2:00 PM, Ken Rockot <rockot@chromium.org> wrote: > >> >> >> On Thu, Dec 10, 2015 at 10:52 AM, Lally Singh <lally@google.com> wrote: >> >>> #2 is correct. >>> >>> #1 works fine as long as you have a trivial STARTLS protocol. For XMPP, >>> we'd need to speak both read() and onReceive for the XMPP stack. Two >>> different APIs, but you can bridge them with a buffer. We've got some >>> server-specific hacks to work right now. It's a nasty pain point for us. >>> >>> I don't know if node compatibility was ever a goal for this API -- there >>> are a *lot* of handy libraries you can use in a chrome app, but it's >>> tougher with the different pause semantics between the two apis. >>> >>> Anyways, I'm spent. If these aren't compelling, then I'll dump the CL >>> and push the test infrastructure in here to the other backpressure CL (I >>> wrote it for both). >>> >> >> Sorry this is being drawn out so much. It's helpful to understand the >> requirements here in more detail. I don't mean to be obtuse, I just want to >> make sure we have the right API. If start-paused is still painful, it's not >> a good option. Server-specific hacks sound painful. >> >> >>> >>> >>> >>> Cheers, >>> $ ls >>> >>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>> Engineering | lally@google.com >>> >>> >>> On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> wrote: >>> >>>> I'm happy to schedule a VC, but I've read the document, and we've >>>> discussed alternative ways of addressing both the secure socket flow and >>>> the back-pressure problem, neither of which seem to require another layer >>>> of implementation-level buffering to be solved. To summarize the >>>> conclusions I drew from that discussion: >>>> >>>> 1. If creating a socket in a paused state helps make secure socket >>>> flow less hacky to implement (seems like it does?), then the details of >>>> set-paused behavior seem largely inconsequential to me and its "broken"ness >>>> is rather subjective. >>>> 2. Back-pressure can be addressed efficiently with a per-socket >>>> option to configure an upper bound on the size of the receive-event backlog. >>>> >>>> My assumptions around #1 may be invalid. Does managing TLS setup with a >>>> manual read() API (prior to initiating TLS and unpausing) add more >>>> complexity than I'm imagining? >>>> >>>> >>>> On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> wrote: >>>> >>>>> I believe Lally shared the the proposed design >>>>> <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite >>>>> a while ago and it was discussed a bunch then. My understanding is that its purpose >>>>> is to fix two bad things with the current API: set-paused is broken, >>>>> which means that many secure socket flows need to be hacked/don't work, and >>>>> there is no back-pressure, which means you have to implement a very >>>>> inefficient version in JS. Having back-pressure and secure sockets behave >>>>> properly seems good to me, and is certainly what I'd expect of a tcp socket >>>>> library. I suspect there is some basic misunderstanding here, maybe a >>>>> short VC would help? >>>>> >>>>> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: >>>>> >>>>>> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >>>>>> > In avoiding this, I think we'd just be making clients implement the >>>>>> same >>>>>> > buffer in their code. >>>>>> >>>>>> > Which is sad, because the problem is ours and we're doing a poor >>>>>> job here. >>>>>> > Node.js implements a similar pause API and does a better job here. >>>>>> >>>>>> I would prefer our APIs remain minimalist in their design. >>>>>> Auto-polling at >>>>>> the >>>>>> implementation layer adds value in that it avoids some latency >>>>>> imposed by >>>>>> the JS >>>>>> event loop. I don't a similar level of value being added by this >>>>>> buffering >>>>>> behavior. >>>>>> >>>>>> Since pause can't actually stop a pending read from completing or >>>>>> data from >>>>>> arriving, this buffering only artificially hides an event from the >>>>>> client. >>>>>> Why >>>>>> is that better than firing the event and letting the client decide >>>>>> what to >>>>>> do? >>>>>> I'm certain that not every client is averse to receiving and >>>>>> processing some >>>>>> data after issuing a pause. >>>>>> >>>>>> >>>>>> > This CL is a buffer, some plumbing, and test cases. If it's too >>>>>> big or >>>>>> > there are ways you can see to make it smaller, I'm happy to break >>>>>> it up or >>>>>> > adjust it. >>>>>> >>>>>> > We (uProxy) use the socket api quite a bit, and implement quite a >>>>>> bit >>>>>> > atop: >>>>>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been >>>>>> hitting >>>>>> > issues with sockets (basically, pausing and backpressure) for a >>>>>> while, and >>>>>> > I'm quite eager to fix them. The workarounds in code are quite >>>>>> painful >>>>>> > and >>>>>> > cost us substantial amounts of memory and performance. >>>>>> >>>>>> >>>>>> >>>>>> > Cheers, >>>>>> > $ ls >>>>>> >>>>>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>> Engineering | >>>>>> > lally@google.com >>>>>> >>>>>> >>>>>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >>>>>> wrote: >>>>>> >>>>>> > > I'm not convinced it needs to be fixed as long as it's understood >>>>>> that >>>>>> > > there's no guarantee that data won't arrive after pause. I can >>>>>> imagine >>>>>> > > cases where it's still useful without that guarantee, but your >>>>>> case >>>>>> > isn't >>>>>> > > one of them. >>>>>> > > >>>>>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> >>>>>> wrote: >>>>>> > > >>>>>> > >> I'm happy with an N > 1 approach. >>>>>> > >> >>>>>> > >> I'm not sure how else to fix the currently-broken setPaused() >>>>>> api other >>>>>> > >> than buffering. >>>>>> > >> >>>>>> > >> Cheers, >>>>>> > >> $ ls >>>>>> > >> >>>>>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>> > Engineering | >>>>>> > >> lally@google.com >>>>>> > >> >>>>>> > >> >>>>>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <rockot@chromium.org >>>>>> > >>>>>> > wrote: >>>>>> > >> >>>>>> > >>> I think it would be reasonable (for #5) to have a max pending >>>>>> receive >>>>>> > >>> option. It would work generally as you describe (don't read >>>>>> until >>>>>> > event(s) >>>>>> > >>> have been acked by JS), it would just support N > 1 outstanding >>>>>> > receive >>>>>> > >>> events. >>>>>> > >>> >>>>>> > >>> Re #4: Doh... right, I see, because we can't just cancel an >>>>>> > outstanding >>>>>> > >>> read syscall. In that case I'd like to just continue on a path >>>>>> that >>>>>> doesn't >>>>>> > >>> require us to solve that problem, which I think start-paused >>>>>> does. :) >>>>>> > >>> >>>>>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> >>>>>> wrote: >>>>>> > >>> >>>>>> > >>>> I'm happy with a "start paused" option, and a read() method to >>>>>> > access a >>>>>> > >>>> paused stream. I think that's a natural extension of the >>>>>> current >>>>>> > API. >>>>>> > >>>> >>>>>> > >>>> For #4: The problem with just deferring the setPaused() >>>>>> callback is >>>>>> > >>>> that data may not be coming back anytime soon, so that >>>>>> callback can >>>>>> > sit >>>>>> and >>>>>> > >>>> wait a long time. I'm not sure how to get around that without >>>>>> buffering. >>>>>> > >>>> >>>>>> > >>>> For #5: any thoughts on what kind of throttle specification >>>>>> that'd >>>>>> > be? >>>>>> > >>>> A max-outstanding-JS events count, where you can specify a >>>>>> count > >>>>>> > 1, can >>>>>> > >>>> keep throughput high. But if we stop talking in terms of >>>>>> > outstanding JS >>>>>> > >>>> events, we have to find another way to maintain sensitivity to >>>>>> CPU >>>>>> pressure. >>>>>> > >>>> >>>>>> > >>>> >>>>>> > >>>> >>>>>> > >>>> Cheers, >>>>>> > >>>> $ ls >>>>>> > >>>> >>>>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>> > >>>> Engineering | lally@google.com >>>>>> > >>>> >>>>>> > >>>> >>>>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote: >>>>>> > >>>> >>>>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>>>> > >>>>> >>>>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>>>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >>>>>> implementation. >>>>>> > >>>>>> setPause() >>>>>> > >>>>>> >>>>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the >>>>>> socket, >>>>>> > >>>>> and then >>>>>> > >>>>> issues onReceive events back to JS when those Read()s >>>>>> complete. >>>>>> > >>>>> >>>>>> > >>>>>> > >>>>>> > >>>>>> > But, when pausing, any issued Read() continues to live on. >>>>>> > >>>>>> Currently, that >>>>>> > >>>>>> >>>>>> > >>>>> means that another onReceive() is likely to come, even when >>>>>> the >>>>>> > >>>>> socket's paused. >>>>>> > >>>>> This change buffers the data that comes back from Read() for >>>>>> > sending >>>>>> > >>>>> later, >>>>>> > >>>>> when the socket is un-paused. >>>>>> > >>>>> >>>>>> > >>>>>> > >>>>>> > >>>>>> > I've written up the issues here: >>>>>> > >>>>>> >>>>>> > >>>>> >>>>>> > >>>>> >>>>>> >>>>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>>>> > >>>>> . >>>>>> > >>>>> We don't do backpressure here (that's in a follow-up CL), >>>>>> but some >>>>>> > of >>>>>> > >>>>> the >>>>>> > >>>>> testing support needed for that is included here. >>>>>> > >>>>> >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> > Please take a look, and let me know if you have any >>>>>> questions. >>>>>> > I'm >>>>>> > >>>>>> happy to >>>>>> > >>>>>> >>>>>> > >>>>> talk about this. >>>>>> > >>>>> >>>>>> > >>>>>> > >>>>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in >>>>>> > >>>>>> reviewing this CL. >>>>>> > >>>>>> >>>>>> > >>>>> >>>>>> > >>>>> This is an unfortunate problem and I agree that we need to >>>>>> fix it, >>>>>> > but >>>>>> > >>>>>> this >>>>>> > >>>>>> >>>>>> > >>>>> solution seems fairly high in complexity to me. Here is my >>>>>> > >>>>> counter-proposal for >>>>>> > >>>>> what we might do: >>>>>> > >>>>> >>>>>> > >>>>> 1. Add a read() API that works in a new "manual mode" only >>>>>> > >>>>>> 2. Add an option on socket creation to create the socket >>>>>> in >>>>>> > >>>>>> "manual mode" - >>>>>> > >>>>>> >>>>>> > >>>>> i.e. no auto-polling for receive >>>>>> > >>>>> >>>>>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. >>>>>> to leave >>>>>> > >>>>>> "manual >>>>>> > >>>>>> >>>>>> > >>>>> mode"). NO API for switching back to manual mode, because >>>>>> this is >>>>>> > >>>>> unnecessary >>>>>> > >>>>> and a source of much complexity here. >>>>>> > >>>>> >>>>>> > >>>>> Separately we could also address two (IMHO) lower priority >>>>>> issues: >>>>>> > >>>>>> >>>>>> > >>>>> >>>>>> > >>>>> 4. Defer the setPaused callback until the receive queue is >>>>>> > flushed, >>>>>> > >>>>>> just >>>>>> > >>>>>> >>>>>> > >>>>> because this feels like the right thing to do. >>>>>> > >>>>> >>>>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll mode, >>>>>> to >>>>>> > avoid >>>>>> > >>>>>> the >>>>>> > >>>>>> >>>>>> > >>>>> over-eager reading problem. >>>>>> > >>>>> >>>>>> > >>>>> I think adding 1-3 would be lower complexity than what you >>>>>> have >>>>>> > here, >>>>>> > >>>>>> AFAICT >>>>>> > >>>>>> >>>>>> > >>>>> it would address all of your needs, and it doesn't actually >>>>>> change >>>>>> > any >>>>>> > >>>>> behavior >>>>>> > >>>>> of existing API uses in the wild (for better or worse). >>>>>> > >>>>> >>>>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible >>>>>> throttling >>>>>> > >>>>>> solution >>>>>> > >>>>>> >>>>>> > >>>>> than strictly tying read call frequency to the JS event loop. >>>>>> > >>>>> >>>>>> > >>>>> WDYT? >>>>>> > >>>>>> >>>>>> > >>>>> >>>>>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, >>>>>> we have >>>>>> > a >>>>>> > >>>>> "start >>>>>> > >>>>> paused" option. Then read() is, as you suggest, a way of >>>>>> reading >>>>>> > from >>>>>> > >>>>> the socket >>>>>> > >>>>> while paused. The main difference is that we can avoid trying >>>>>> to >>>>>> > >>>>> resolve the >>>>>> > >>>>> complexity of pausing at a very specific place in the stream, >>>>>> by >>>>>> > >>>>> making it >>>>>> > >>>>> possible to start the socket in a paused state. >>>>>> > >>>>> >>>>>> > >>>>> The trade-off is that you have to do manual reads until >>>>>> you're ready >>>>>> > >>>>> to initiate >>>>>> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >>>>>> > >>>>> >>>>>> > >>>>> >>>>>> > >>>>> https://codereview.chromium.org/494573002/ >>>>>> > >>>>> >>>>>> > >>>> >>>>>> > >>>> >>>>>> > >>> >>>>>> > >> >>>>>> > > >>>>>> >>>>>> > -- >>>>>> > You received this message because you are subscribed to the Google >>>>>> Groups >>>>>> "Chromium-reviews" group. >>>>>> > To unsubscribe from this group and stop receiving emails from it, >>>>>> send an >>>>>> email to chromium-reviews+unsubscribe@chromium.org. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/494573002/ >>>>>> >>>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Reading through this thread, it seems the main concern is the complexity of
the current proposed CL vs the benefit of having a stronger setPaused
guarantee. So, I am assuming that if the CL was dead simple, there would be
no strong argument against changing the "setPaused" behavior, right?
So, if one of the main concern is complexity of the current CL, have you
considered making a more local change, i.e. only
changing tcp_socket_event_dispatcher.cc?
I have not looked at the code too closely lately, but it seems that you
could
- Add a single io_buffer "buffered_data" to ResumableTCPSocket
- In TCPSocketEventDispatcher::ReadCallback
- If the ResumableTCPSocket is paused, transfer the io_buffer from
the callback to "ResumableTCPSocket::buffered_data"
- Note you should be able to DCHECK here that this happens at most
once, and that you don't assign buffered_data twice
- If the socket is not paused, do the usual (post
a SOCKETS_TCP_ON_RECEIVE message)
- In TCPSocketEventDispatcher::StartRead
- After the "paused()" check,
- If ResumableTCPSocket::buffered_data is set, post
a SOCKETS_TCP_ON_RECEIVE message and reset buffered_data to NULL.
- Then do the usual thing (start an async read)
This would really be only 3 (simple?) local changes (highlighted parts). I
am not 100% sure this would work, as the state machine is becoming quite
complex, but maybe it is worth a try.
That said, unless I am missing something, both solutions rely on the fact
that the callback from setPaused will be invoked after *all* pending
onReceive events are invoked (this can happen if the JS client does not
have the time to process onReceive events fast enough). Do we know this is
guaranteed by the Extension binding mechanism? If not, then this change
alone would not improve things to a meaningful extend, and the (future) CL
addressing the backpressure issue would be required for this to be
effective.
If I am totally off track, apologies, and feel free to ignore this :)
Renaud
On Mon, Dec 14, 2015 at 9:38 AM, Lucas Dixon <ldixon@google.com> wrote:
> Reading a set of bytes can be important for STARTTLS flows, especially
> because I don't think, in general, you get a guarantee about what each
> packet contains. Without precise control of how much gets read, you can't
> be sure that you pause/read the right amount before encryption starts.
>
> On Mon, 14 Dec 2015 at 11:46 Lally Singh <lally@google.com> wrote:
>
>> I asked around the team for where we were having and seeing pain points:
>>
>> * We use setpaused() for flow control. Even with backpressure, we still
>> have to stop the incoming flow sometimes to let our internal queues drain
>> out. The additional slop of an additional onReceive after we've already
>> determined that we want to stop the influx reduces our control over the
>> flow rates. We have a lot of simultaneous connections, so this adds up.
>>
>> * An external group doing IMAP:
>>
https://github.com/MobileChromeApps/mobile-chrome-apps/issues/269#issuecommen...
-
>> their workaround counts incoming onReceive packets and times a transmit
>> right after a pause to get the sequencing right. But there are a bunch of
>> API users on that thread, it's tough to tell what'll work for all of them.
>>
>> * From a team member here:
>>
>>> Yeah, this was a pain for me - it took a while to figure out what I was
>>> doing wrong and realize it was an issue with the API rather than my code.
>>> Then to fix it, I had to write a new "prepareSecure" method for both
>>> freedom-for-firefox (a no-op) and freedom-for-chrome (calls .pause), plus
>>> the common API definition in freedomjs and call it 1 read prior to wanting
>>> to secure the socket.
>>>
>>> In the case of XMPP starttls flow we were able to work around this
>>> because XMPP works like this:
>>>
>>> 1. client sends init message to server in cleartext
>>> 2. server sends response in cleartext
>>> 3. client sends some more data in cleartext
>>> 4. server replies with starttls command in cleartext
>>> 5. client upgrades the socket to TCP
>>>
>>> Because this flow is predictable we can call prepareSecure (pause)
>>> before step 3 as we know there will be one more read coming up, and then at
>>> step 5 we can actually call .secure. But there may be other protocols
>>> where this doesn't work - the user might call .pause yet if the server
>>> doesn't reply with anything then .pause will never fulfill and they can
>>> never call .secure
>>>
>> [ Note that this is all talking XMPP, which is nontrival to implement on
>> both read() and onReceive ]
>>
>> And we've got 19 stars on one of the bugs on this (crbug.com/403076) and
>> 23 on a duplicate (crbug.com/467677) -- but I couldn't tell you about
>> how many of those are overlaps.
>>
>> In contrast, combining this setPause() change with the backpressure CL,
>> and setting N=1 (although I'll amend that CL to allow N to be adjustable)
>> gives you completely precise socket reads. You can pause a socket during
>> an onReceive event handler and it'll take effect immediately. That's very
>> useful if you want to read the next K bytes to determine what to do next ,
>> set your onReceive event handlers to different functions, depending on what
>> value you got for the K bytes, and then unpause the socket.
>>
>>
>>
>>
>>
>> Cheers,
>> $ ls
>>
>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering |
>> lally@google.com
>>
>>
>> On Thu, Dec 10, 2015 at 2:00 PM, Ken Rockot <rockot@chromium.org> wrote:
>>
>>>
>>>
>>> On Thu, Dec 10, 2015 at 10:52 AM, Lally Singh <lally@google.com> wrote:
>>>
>>>> #2 is correct.
>>>>
>>>> #1 works fine as long as you have a trivial STARTLS protocol. For
>>>> XMPP, we'd need to speak both read() and onReceive for the XMPP stack.
Two
>>>> different APIs, but you can bridge them with a buffer. We've got some
>>>> server-specific hacks to work right now. It's a nasty pain point for us.
>>>>
>>>> I don't know if node compatibility was ever a goal for this API --
>>>> there are a *lot* of handy libraries you can use in a chrome app, but it's
>>>> tougher with the different pause semantics between the two apis.
>>>>
>>>> Anyways, I'm spent. If these aren't compelling, then I'll dump the CL
>>>> and push the test infrastructure in here to the other backpressure CL (I
>>>> wrote it for both).
>>>>
>>>
>>> Sorry this is being drawn out so much. It's helpful to understand the
>>> requirements here in more detail. I don't mean to be obtuse, I just want to
>>> make sure we have the right API. If start-paused is still painful, it's not
>>> a good option. Server-specific hacks sound painful.
>>>
>>>
>>>>
>>>>
>>>>
>>>> Cheers,
>>>> $ ls
>>>>
>>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> |
>>>> Engineering | lally@google.com
>>>>
>>>>
>>>> On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> wrote:
>>>>
>>>>> I'm happy to schedule a VC, but I've read the document, and we've
>>>>> discussed alternative ways of addressing both the secure socket flow and
>>>>> the back-pressure problem, neither of which seem to require another layer
>>>>> of implementation-level buffering to be solved. To summarize the
>>>>> conclusions I drew from that discussion:
>>>>>
>>>>> 1. If creating a socket in a paused state helps make secure socket
>>>>> flow less hacky to implement (seems like it does?), then the details
of
>>>>> set-paused behavior seem largely inconsequential to me and its
"broken"ness
>>>>> is rather subjective.
>>>>> 2. Back-pressure can be addressed efficiently with a per-socket
>>>>> option to configure an upper bound on the size of the receive-event
backlog.
>>>>>
>>>>> My assumptions around #1 may be invalid. Does managing TLS setup with
>>>>> a manual read() API (prior to initiating TLS and unpausing) add more
>>>>> complexity than I'm imagining?
>>>>>
>>>>>
>>>>> On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> wrote:
>>>>>
>>>>>> I believe Lally shared the the proposed design
>>>>>>
<https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/>
quite
>>>>>> a while ago and it was discussed a bunch then. My understanding is that
its purpose
>>>>>> is to fix two bad things with the current API: set-paused is broken,
>>>>>> which means that many secure socket flows need to be hacked/don't work,
and
>>>>>> there is no back-pressure, which means you have to implement a very
>>>>>> inefficient version in JS. Having back-pressure and secure sockets
behave
>>>>>> properly seems good to me, and is certainly what I'd expect of a tcp
socket
>>>>>> library. I suspect there is some basic misunderstanding here, maybe a
>>>>>> short VC would help?
>>>>>>
>>>>>> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote:
>>>>>>
>>>>>>> On 2015/12/07 at 20:12:49, chromium-reviews wrote:
>>>>>>> > In avoiding this, I think we'd just be making clients implement
>>>>>>> the same
>>>>>>> > buffer in their code.
>>>>>>>
>>>>>>> > Which is sad, because the problem is ours and we're doing a poor
>>>>>>> job here.
>>>>>>> > Node.js implements a similar pause API and does a better job here.
>>>>>>>
>>>>>>> I would prefer our APIs remain minimalist in their design.
>>>>>>> Auto-polling at
>>>>>>> the
>>>>>>> implementation layer adds value in that it avoids some latency
>>>>>>> imposed by
>>>>>>> the JS
>>>>>>> event loop. I don't a similar level of value being added by this
>>>>>>> buffering
>>>>>>> behavior.
>>>>>>>
>>>>>>> Since pause can't actually stop a pending read from completing or
>>>>>>> data from
>>>>>>> arriving, this buffering only artificially hides an event from the
>>>>>>> client.
>>>>>>> Why
>>>>>>> is that better than firing the event and letting the client decide
>>>>>>> what to
>>>>>>> do?
>>>>>>> I'm certain that not every client is averse to receiving and
>>>>>>> processing some
>>>>>>> data after issuing a pause.
>>>>>>>
>>>>>>>
>>>>>>> > This CL is a buffer, some plumbing, and test cases. If it's too
>>>>>>> big or
>>>>>>> > there are ways you can see to make it smaller, I'm happy to break
>>>>>>> it up or
>>>>>>> > adjust it.
>>>>>>>
>>>>>>> > We (uProxy) use the socket api quite a bit, and implement quite a
>>>>>>> bit
>>>>>>> > atop:
>>>>>>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been
>>>>>>> hitting
>>>>>>> > issues with sockets (basically, pausing and backpressure) for a
>>>>>>> while, and
>>>>>>> > I'm quite eager to fix them. The workarounds in code are quite
>>>>>>> painful
>>>>>>> > and
>>>>>>> > cost us substantial amounts of memory and performance.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> > Cheers,
>>>>>>> > $ ls
>>>>>>>
>>>>>>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> |
>>>>>>> Engineering |
>>>>>>> > lally@google.com
>>>>>>>
>>>>>>>
>>>>>>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>> > > I'm not convinced it needs to be fixed as long as it's
>>>>>>> understood that
>>>>>>> > > there's no guarantee that data won't arrive after pause. I can
>>>>>>> imagine
>>>>>>> > > cases where it's still useful without that guarantee, but your
>>>>>>> case
>>>>>>> > isn't
>>>>>>> > > one of them.
>>>>>>> > >
>>>>>>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com>
>>>>>>> wrote:
>>>>>>> > >
>>>>>>> > >> I'm happy with an N > 1 approach.
>>>>>>> > >>
>>>>>>> > >> I'm not sure how else to fix the currently-broken setPaused()
>>>>>>> api other
>>>>>>> > >> than buffering.
>>>>>>> > >>
>>>>>>> > >> Cheers,
>>>>>>> > >> $ ls
>>>>>>> > >>
>>>>>>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> |
>>>>>>> > Engineering |
>>>>>>> > >> lally@google.com
>>>>>>> > >>
>>>>>>> > >>
>>>>>>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot <
>>>>>>> rockot@chromium.org>
>>>>>>> > wrote:
>>>>>>> > >>
>>>>>>> > >>> I think it would be reasonable (for #5) to have a max pending
>>>>>>> receive
>>>>>>> > >>> option. It would work generally as you describe (don't read
>>>>>>> until
>>>>>>> > event(s)
>>>>>>> > >>> have been acked by JS), it would just support N > 1 outstanding
>>>>>>> > receive
>>>>>>> > >>> events.
>>>>>>> > >>>
>>>>>>> > >>> Re #4: Doh... right, I see, because we can't just cancel an
>>>>>>> > outstanding
>>>>>>> > >>> read syscall. In that case I'd like to just continue on a path
>>>>>>> that
>>>>>>> doesn't
>>>>>>> > >>> require us to solve that problem, which I think start-paused
>>>>>>> does. :)
>>>>>>> > >>>
>>>>>>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com>
>>>>>>> wrote:
>>>>>>> > >>>
>>>>>>> > >>>> I'm happy with a "start paused" option, and a read() method to
>>>>>>> > access a
>>>>>>> > >>>> paused stream. I think that's a natural extension of the
>>>>>>> current
>>>>>>> > API.
>>>>>>> > >>>>
>>>>>>> > >>>> For #4: The problem with just deferring the setPaused()
>>>>>>> callback is
>>>>>>> > >>>> that data may not be coming back anytime soon, so that
>>>>>>> callback can
>>>>>>> > sit
>>>>>>> and
>>>>>>> > >>>> wait a long time. I'm not sure how to get around that
>>>>>>> without
>>>>>>> buffering.
>>>>>>> > >>>>
>>>>>>> > >>>> For #5: any thoughts on what kind of throttle specification
>>>>>>> that'd
>>>>>>> > be?
>>>>>>> > >>>> A max-outstanding-JS events count, where you can specify a
>>>>>>> count >
>>>>>>> > 1, can
>>>>>>> > >>>> keep throughput high. But if we stop talking in terms of
>>>>>>> > outstanding JS
>>>>>>> > >>>> events, we have to find another way to maintain sensitivity
>>>>>>> to CPU
>>>>>>> pressure.
>>>>>>> > >>>>
>>>>>>> > >>>>
>>>>>>> > >>>>
>>>>>>> > >>>> Cheers,
>>>>>>> > >>>> $ ls
>>>>>>> > >>>>
>>>>>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> |
>>>>>>> > >>>> Engineering | lally@google.com
>>>>>>> > >>>>
>>>>>>> > >>>>
>>>>>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> wrote:
>>>>>>> > >>>>
>>>>>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote:
>>>>>>> > >>>>>
>>>>>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote:
>>>>>>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause()
>>>>>>> implementation.
>>>>>>> > >>>>>> setPause()
>>>>>>> > >>>>>>
>>>>>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the
>>>>>>> socket,
>>>>>>> > >>>>> and then
>>>>>>> > >>>>> issues onReceive events back to JS when those Read()s
>>>>>>> complete.
>>>>>>> > >>>>>
>>>>>>> > >>>>>> >
>>>>>>> > >>>>>> > But, when pausing, any issued Read() continues to live on.
>>>>>>> > >>>>>> Currently, that
>>>>>>> > >>>>>>
>>>>>>> > >>>>> means that another onReceive() is likely to come, even when
>>>>>>> the
>>>>>>> > >>>>> socket's paused.
>>>>>>> > >>>>> This change buffers the data that comes back from Read() for
>>>>>>> > sending
>>>>>>> > >>>>> later,
>>>>>>> > >>>>> when the socket is un-paused.
>>>>>>> > >>>>>
>>>>>>> > >>>>>> >
>>>>>>> > >>>>>> > I've written up the issues here:
>>>>>>> > >>>>>>
>>>>>>> > >>>>>
>>>>>>> > >>>>>
>>>>>>>
>>>>>>>
https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/
>>>>>>> > >>>>> .
>>>>>>> > >>>>> We don't do backpressure here (that's in a follow-up CL),
>>>>>>> but some
>>>>>>> > of
>>>>>>> > >>>>> the
>>>>>>> > >>>>> testing support needed for that is included here.
>>>>>>> > >>>>>
>>>>>>> > >>>>>> >
>>>>>>> > >>>>>> >
>>>>>>> > >>>>>> > Please take a look, and let me know if you have any
>>>>>>> questions.
>>>>>>> > I'm
>>>>>>> > >>>>>> happy to
>>>>>>> > >>>>>>
>>>>>>> > >>>>> talk about this.
>>>>>>> > >>>>>
>>>>>>> > >>>>>> >
>>>>>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested in
>>>>>>> > >>>>>> reviewing this CL.
>>>>>>> > >>>>>>
>>>>>>> > >>>>>
>>>>>>> > >>>>> This is an unfortunate problem and I agree that we need to
>>>>>>> fix it,
>>>>>>> > but
>>>>>>> > >>>>>> this
>>>>>>> > >>>>>>
>>>>>>> > >>>>> solution seems fairly high in complexity to me. Here is my
>>>>>>> > >>>>> counter-proposal for
>>>>>>> > >>>>> what we might do:
>>>>>>> > >>>>>
>>>>>>> > >>>>> 1. Add a read() API that works in a new "manual mode" only
>>>>>>> > >>>>>> 2. Add an option on socket creation to create the socket
>>>>>>> in
>>>>>>> > >>>>>> "manual mode" -
>>>>>>> > >>>>>>
>>>>>>> > >>>>> i.e. no auto-polling for receive
>>>>>>> > >>>>>
>>>>>>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e.
>>>>>>> to leave
>>>>>>> > >>>>>> "manual
>>>>>>> > >>>>>>
>>>>>>> > >>>>> mode"). NO API for switching back to manual mode, because
>>>>>>> this is
>>>>>>> > >>>>> unnecessary
>>>>>>> > >>>>> and a source of much complexity here.
>>>>>>> > >>>>>
>>>>>>> > >>>>> Separately we could also address two (IMHO) lower priority
>>>>>>> issues:
>>>>>>> > >>>>>>
>>>>>>> > >>>>>
>>>>>>> > >>>>> 4. Defer the setPaused callback until the receive queue is
>>>>>>> > flushed,
>>>>>>> > >>>>>> just
>>>>>>> > >>>>>>
>>>>>>> > >>>>> because this feels like the right thing to do.
>>>>>>> > >>>>>
>>>>>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll
>>>>>>> mode, to
>>>>>>> > avoid
>>>>>>> > >>>>>> the
>>>>>>> > >>>>>>
>>>>>>> > >>>>> over-eager reading problem.
>>>>>>> > >>>>>
>>>>>>> > >>>>> I think adding 1-3 would be lower complexity than what you
>>>>>>> have
>>>>>>> > here,
>>>>>>> > >>>>>> AFAICT
>>>>>>> > >>>>>>
>>>>>>> > >>>>> it would address all of your needs, and it doesn't actually
>>>>>>> change
>>>>>>> > any
>>>>>>> > >>>>> behavior
>>>>>>> > >>>>> of existing API uses in the wild (for better or worse).
>>>>>>> > >>>>>
>>>>>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible
>>>>>>> throttling
>>>>>>> > >>>>>> solution
>>>>>>> > >>>>>>
>>>>>>> > >>>>> than strictly tying read call frequency to the JS event loop.
>>>>>>> > >>>>>
>>>>>>> > >>>>> WDYT?
>>>>>>> > >>>>>>
>>>>>>> > >>>>>
>>>>>>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing,
>>>>>>> we have
>>>>>>> > a
>>>>>>> > >>>>> "start
>>>>>>> > >>>>> paused" option. Then read() is, as you suggest, a way of
>>>>>>> reading
>>>>>>> > from
>>>>>>> > >>>>> the socket
>>>>>>> > >>>>> while paused. The main difference is that we can avoid
>>>>>>> trying to
>>>>>>> > >>>>> resolve the
>>>>>>> > >>>>> complexity of pausing at a very specific place in the
>>>>>>> stream, by
>>>>>>> > >>>>> making it
>>>>>>> > >>>>> possible to start the socket in a paused state.
>>>>>>> > >>>>>
>>>>>>> > >>>>> The trade-off is that you have to do manual reads until
>>>>>>> you're ready
>>>>>>> > >>>>> to initiate
>>>>>>> > >>>>> TLS and unpause, but that doesn't seem so bad to me.
>>>>>>> > >>>>>
>>>>>>> > >>>>>
>>>>>>> > >>>>> https://codereview.chromium.org/494573002/
>>>>>>> > >>>>>
>>>>>>> > >>>>
>>>>>>> > >>>>
>>>>>>> > >>>
>>>>>>> > >>
>>>>>>> > >
>>>>>>>
>>>>>>> > --
>>>>>>> > You received this message because you are subscribed to the Google
>>>>>>> Groups
>>>>>>> "Chromium-reviews" group.
>>>>>>> > To unsubscribe from this group and stop receiving emails from it,
>>>>>>> send an
>>>>>>> email to chromium-reviews+unsubscribe@chromium.org.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> https://codereview.chromium.org/494573002/
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Hi Renaud, Thank you for this -- any ideas on making this simpler help. I don't know how to make that work across the TLS boundary. The TLS socket uses the native Read() api, and the TCPSocketEventDispatcher sits atop of the TLS socket, calling Read() on it to get decrypted data. That's why there are buffering versions of both socket types (TCPClientSocket and StreamSocket) there --- to support both plaintext and TLS sockets. Would it help (Renaud, Ken) if I added a write-up of the implementation changes to the doc? Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Dec 14, 2015 at 2:06 PM, Renaud Paquay <rpaquay@chromium.org> wrote: > Reading through this thread, it seems the main concern is the complexity > of the current proposed CL vs the benefit of having a stronger setPaused > guarantee. So, I am assuming that if the CL was dead simple, there would be > no strong argument against changing the "setPaused" behavior, right? > > So, if one of the main concern is complexity of the current CL, have you > considered making a more local change, i.e. only > changing tcp_socket_event_dispatcher.cc? > > I have not looked at the code too closely lately, but it seems that you > could > > - Add a single io_buffer "buffered_data" to ResumableTCPSocket > - In TCPSocketEventDispatcher::ReadCallback > - If the ResumableTCPSocket is paused, transfer the io_buffer from > the callback to "ResumableTCPSocket::buffered_data" > - Note you should be able to DCHECK here that this happens at > most once, and that you don't assign buffered_data twice > - If the socket is not paused, do the usual (post > a SOCKETS_TCP_ON_RECEIVE message) > - In TCPSocketEventDispatcher::StartRead > - After the "paused()" check, > - If ResumableTCPSocket::buffered_data is set, post > a SOCKETS_TCP_ON_RECEIVE message and reset buffered_data to NULL. > - Then do the usual thing (start an async read) > > This would really be only 3 (simple?) local changes (highlighted parts). I > am not 100% sure this would work, as the state machine is becoming quite > complex, but maybe it is worth a try. > > That said, unless I am missing something, both solutions rely on the fact > that the callback from setPaused will be invoked after *all* pending > onReceive events are invoked (this can happen if the JS client does not > have the time to process onReceive events fast enough). Do we know this is > guaranteed by the Extension binding mechanism? If not, then this change > alone would not improve things to a meaningful extend, and the (future) CL > addressing the backpressure issue would be required for this to be > effective. > > If I am totally off track, apologies, and feel free to ignore this :) > > Renaud > > > > On Mon, Dec 14, 2015 at 9:38 AM, Lucas Dixon <ldixon@google.com> wrote: > >> Reading a set of bytes can be important for STARTTLS flows, especially >> because I don't think, in general, you get a guarantee about what each >> packet contains. Without precise control of how much gets read, you can't >> be sure that you pause/read the right amount before encryption starts. >> >> On Mon, 14 Dec 2015 at 11:46 Lally Singh <lally@google.com> wrote: >> >>> I asked around the team for where we were having and seeing pain points: >>> >>> * We use setpaused() for flow control. Even with backpressure, we >>> still have to stop the incoming flow sometimes to let our internal queues >>> drain out. The additional slop of an additional onReceive after we've >>> already determined that we want to stop the influx reduces our control over >>> the flow rates. We have a lot of simultaneous connections, so this adds up. >>> >>> * An external group doing IMAP: >>> https://github.com/MobileChromeApps/mobile-chrome-apps/issues/269#issuecommen... - >>> their workaround counts incoming onReceive packets and times a transmit >>> right after a pause to get the sequencing right. But there are a bunch of >>> API users on that thread, it's tough to tell what'll work for all of them. >>> >>> * From a team member here: >>> >>>> Yeah, this was a pain for me - it took a while to figure out what I was >>>> doing wrong and realize it was an issue with the API rather than my code. >>>> Then to fix it, I had to write a new "prepareSecure" method for both >>>> freedom-for-firefox (a no-op) and freedom-for-chrome (calls .pause), plus >>>> the common API definition in freedomjs and call it 1 read prior to wanting >>>> to secure the socket. >>>> >>>> In the case of XMPP starttls flow we were able to work around this >>>> because XMPP works like this: >>>> >>>> 1. client sends init message to server in cleartext >>>> 2. server sends response in cleartext >>>> 3. client sends some more data in cleartext >>>> 4. server replies with starttls command in cleartext >>>> 5. client upgrades the socket to TCP >>>> >>>> Because this flow is predictable we can call prepareSecure (pause) >>>> before step 3 as we know there will be one more read coming up, and then at >>>> step 5 we can actually call .secure. But there may be other protocols >>>> where this doesn't work - the user might call .pause yet if the server >>>> doesn't reply with anything then .pause will never fulfill and they can >>>> never call .secure >>>> >>> [ Note that this is all talking XMPP, which is nontrival to implement >>> on both read() and onReceive ] >>> >>> And we've got 19 stars on one of the bugs on this (crbug.com/403076) >>> and 23 on a duplicate (crbug.com/467677) -- but I couldn't tell you >>> about how many of those are overlaps. >>> >>> In contrast, combining this setPause() change with the backpressure CL, >>> and setting N=1 (although I'll amend that CL to allow N to be adjustable) >>> gives you completely precise socket reads. You can pause a socket during >>> an onReceive event handler and it'll take effect immediately. That's very >>> useful if you want to read the next K bytes to determine what to do next , >>> set your onReceive event handlers to different functions, depending on what >>> value you got for the K bytes, and then unpause the socket. >>> >>> >>> >>> >>> >>> Cheers, >>> $ ls >>> >>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>> Engineering | lally@google.com >>> >>> >>> On Thu, Dec 10, 2015 at 2:00 PM, Ken Rockot <rockot@chromium.org> wrote: >>> >>>> >>>> >>>> On Thu, Dec 10, 2015 at 10:52 AM, Lally Singh <lally@google.com> wrote: >>>> >>>>> #2 is correct. >>>>> >>>>> #1 works fine as long as you have a trivial STARTLS protocol. For >>>>> XMPP, we'd need to speak both read() and onReceive for the XMPP stack. Two >>>>> different APIs, but you can bridge them with a buffer. We've got some >>>>> server-specific hacks to work right now. It's a nasty pain point for us. >>>>> >>>>> I don't know if node compatibility was ever a goal for this API -- >>>>> there are a *lot* of handy libraries you can use in a chrome app, but it's >>>>> tougher with the different pause semantics between the two apis. >>>>> >>>>> Anyways, I'm spent. If these aren't compelling, then I'll dump the CL >>>>> and push the test infrastructure in here to the other backpressure CL (I >>>>> wrote it for both). >>>>> >>>> >>>> Sorry this is being drawn out so much. It's helpful to understand the >>>> requirements here in more detail. I don't mean to be obtuse, I just want to >>>> make sure we have the right API. If start-paused is still painful, it's not >>>> a good option. Server-specific hacks sound painful. >>>> >>>> >>>>> >>>>> >>>>> >>>>> Cheers, >>>>> $ ls >>>>> >>>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>> Engineering | lally@google.com >>>>> >>>>> >>>>> On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> >>>>> wrote: >>>>> >>>>>> I'm happy to schedule a VC, but I've read the document, and we've >>>>>> discussed alternative ways of addressing both the secure socket flow and >>>>>> the back-pressure problem, neither of which seem to require another layer >>>>>> of implementation-level buffering to be solved. To summarize the >>>>>> conclusions I drew from that discussion: >>>>>> >>>>>> 1. If creating a socket in a paused state helps make secure >>>>>> socket flow less hacky to implement (seems like it does?), then the details >>>>>> of set-paused behavior seem largely inconsequential to me and its >>>>>> "broken"ness is rather subjective. >>>>>> 2. Back-pressure can be addressed efficiently with a per-socket >>>>>> option to configure an upper bound on the size of the receive-event backlog. >>>>>> >>>>>> My assumptions around #1 may be invalid. Does managing TLS setup with >>>>>> a manual read() API (prior to initiating TLS and unpausing) add more >>>>>> complexity than I'm imagining? >>>>>> >>>>>> >>>>>> On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> >>>>>> wrote: >>>>>> >>>>>>> I believe Lally shared the the proposed design >>>>>>> <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite >>>>>>> a while ago and it was discussed a bunch then. My understanding is that its purpose >>>>>>> is to fix two bad things with the current API: set-paused is >>>>>>> broken, which means that many secure socket flows need to be hacked/don't >>>>>>> work, and there is no back-pressure, which means you have to implement a >>>>>>> very inefficient version in JS. Having back-pressure and secure sockets >>>>>>> behave properly seems good to me, and is certainly what I'd expect of a tcp >>>>>>> socket library. I suspect there is some basic misunderstanding here, maybe >>>>>>> a short VC would help? >>>>>>> >>>>>>> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: >>>>>>> >>>>>>>> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >>>>>>>> > In avoiding this, I think we'd just be making clients implement >>>>>>>> the same >>>>>>>> > buffer in their code. >>>>>>>> >>>>>>>> > Which is sad, because the problem is ours and we're doing a poor >>>>>>>> job here. >>>>>>>> > Node.js implements a similar pause API and does a better job here. >>>>>>>> >>>>>>>> I would prefer our APIs remain minimalist in their design. >>>>>>>> Auto-polling at >>>>>>>> the >>>>>>>> implementation layer adds value in that it avoids some latency >>>>>>>> imposed by >>>>>>>> the JS >>>>>>>> event loop. I don't a similar level of value being added by this >>>>>>>> buffering >>>>>>>> behavior. >>>>>>>> >>>>>>>> Since pause can't actually stop a pending read from completing or >>>>>>>> data from >>>>>>>> arriving, this buffering only artificially hides an event from the >>>>>>>> client. >>>>>>>> Why >>>>>>>> is that better than firing the event and letting the client decide >>>>>>>> what to >>>>>>>> do? >>>>>>>> I'm certain that not every client is averse to receiving and >>>>>>>> processing some >>>>>>>> data after issuing a pause. >>>>>>>> >>>>>>>> >>>>>>>> > This CL is a buffer, some plumbing, and test cases. If it's too >>>>>>>> big or >>>>>>>> > there are ways you can see to make it smaller, I'm happy to break >>>>>>>> it up or >>>>>>>> > adjust it. >>>>>>>> >>>>>>>> > We (uProxy) use the socket api quite a bit, and implement quite a >>>>>>>> bit >>>>>>>> > atop: >>>>>>>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been >>>>>>>> hitting >>>>>>>> > issues with sockets (basically, pausing and backpressure) for a >>>>>>>> while, and >>>>>>>> > I'm quite eager to fix them. The workarounds in code are quite >>>>>>>> painful >>>>>>>> > and >>>>>>>> > cost us substantial amounts of memory and performance. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> > Cheers, >>>>>>>> > $ ls >>>>>>>> >>>>>>>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>>>> Engineering | >>>>>>>> > lally@google.com >>>>>>>> >>>>>>>> >>>>>>>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>> > > I'm not convinced it needs to be fixed as long as it's >>>>>>>> understood that >>>>>>>> > > there's no guarantee that data won't arrive after pause. I can >>>>>>>> imagine >>>>>>>> > > cases where it's still useful without that guarantee, but your >>>>>>>> case >>>>>>>> > isn't >>>>>>>> > > one of them. >>>>>>>> > > >>>>>>>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> >>>>>>>> wrote: >>>>>>>> > > >>>>>>>> > >> I'm happy with an N > 1 approach. >>>>>>>> > >> >>>>>>>> > >> I'm not sure how else to fix the currently-broken setPaused() >>>>>>>> api other >>>>>>>> > >> than buffering. >>>>>>>> > >> >>>>>>>> > >> Cheers, >>>>>>>> > >> $ ls >>>>>>>> > >> >>>>>>>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>>>> > Engineering | >>>>>>>> > >> lally@google.com >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot < >>>>>>>> rockot@chromium.org> >>>>>>>> > wrote: >>>>>>>> > >> >>>>>>>> > >>> I think it would be reasonable (for #5) to have a max pending >>>>>>>> receive >>>>>>>> > >>> option. It would work generally as you describe (don't read >>>>>>>> until >>>>>>>> > event(s) >>>>>>>> > >>> have been acked by JS), it would just support N > 1 >>>>>>>> outstanding >>>>>>>> > receive >>>>>>>> > >>> events. >>>>>>>> > >>> >>>>>>>> > >>> Re #4: Doh... right, I see, because we can't just cancel an >>>>>>>> > outstanding >>>>>>>> > >>> read syscall. In that case I'd like to just continue on a >>>>>>>> path that >>>>>>>> doesn't >>>>>>>> > >>> require us to solve that problem, which I think start-paused >>>>>>>> does. :) >>>>>>>> > >>> >>>>>>>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh <lally@google.com> >>>>>>>> wrote: >>>>>>>> > >>> >>>>>>>> > >>>> I'm happy with a "start paused" option, and a read() method >>>>>>>> to >>>>>>>> > access a >>>>>>>> > >>>> paused stream. I think that's a natural extension of the >>>>>>>> current >>>>>>>> > API. >>>>>>>> > >>>> >>>>>>>> > >>>> For #4: The problem with just deferring the setPaused() >>>>>>>> callback is >>>>>>>> > >>>> that data may not be coming back anytime soon, so that >>>>>>>> callback can >>>>>>>> > sit >>>>>>>> and >>>>>>>> > >>>> wait a long time. I'm not sure how to get around that >>>>>>>> without >>>>>>>> buffering. >>>>>>>> > >>>> >>>>>>>> > >>>> For #5: any thoughts on what kind of throttle specification >>>>>>>> that'd >>>>>>>> > be? >>>>>>>> > >>>> A max-outstanding-JS events count, where you can specify a >>>>>>>> count > >>>>>>>> > 1, can >>>>>>>> > >>>> keep throughput high. But if we stop talking in terms of >>>>>>>> > outstanding JS >>>>>>>> > >>>> events, we have to find another way to maintain sensitivity >>>>>>>> to CPU >>>>>>>> pressure. >>>>>>>> > >>>> >>>>>>>> > >>>> >>>>>>>> > >>>> >>>>>>>> > >>>> Cheers, >>>>>>>> > >>>> $ ls >>>>>>>> > >>>> >>>>>>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>>>> > >>>> Engineering | lally@google.com >>>>>>>> > >>>> >>>>>>>> > >>>> >>>>>>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> >>>>>>>> wrote: >>>>>>>> > >>>> >>>>>>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>>>>>> > >>>>> >>>>>>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>>>>>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >>>>>>>> implementation. >>>>>>>> > >>>>>> setPause() >>>>>>>> > >>>>>> >>>>>>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on the >>>>>>>> socket, >>>>>>>> > >>>>> and then >>>>>>>> > >>>>> issues onReceive events back to JS when those Read()s >>>>>>>> complete. >>>>>>>> > >>>>> >>>>>>>> > >>>>>> > >>>>>>>> > >>>>>> > But, when pausing, any issued Read() continues to live >>>>>>>> on. >>>>>>>> > >>>>>> Currently, that >>>>>>>> > >>>>>> >>>>>>>> > >>>>> means that another onReceive() is likely to come, even when >>>>>>>> the >>>>>>>> > >>>>> socket's paused. >>>>>>>> > >>>>> This change buffers the data that comes back from Read() >>>>>>>> for >>>>>>>> > sending >>>>>>>> > >>>>> later, >>>>>>>> > >>>>> when the socket is un-paused. >>>>>>>> > >>>>> >>>>>>>> > >>>>>> > >>>>>>>> > >>>>>> > I've written up the issues here: >>>>>>>> > >>>>>> >>>>>>>> > >>>>> >>>>>>>> > >>>>> >>>>>>>> >>>>>>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>>>>>> > >>>>> . >>>>>>>> > >>>>> We don't do backpressure here (that's in a follow-up CL), >>>>>>>> but some >>>>>>>> > of >>>>>>>> > >>>>> the >>>>>>>> > >>>>> testing support needed for that is included here. >>>>>>>> > >>>>> >>>>>>>> > >>>>>> > >>>>>>>> > >>>>>> > >>>>>>>> > >>>>>> > Please take a look, and let me know if you have any >>>>>>>> questions. >>>>>>>> > I'm >>>>>>>> > >>>>>> happy to >>>>>>>> > >>>>>> >>>>>>>> > >>>>> talk about this. >>>>>>>> > >>>>> >>>>>>>> > >>>>>> > >>>>>>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested >>>>>>>> in >>>>>>>> > >>>>>> reviewing this CL. >>>>>>>> > >>>>>> >>>>>>>> > >>>>> >>>>>>>> > >>>>> This is an unfortunate problem and I agree that we need to >>>>>>>> fix it, >>>>>>>> > but >>>>>>>> > >>>>>> this >>>>>>>> > >>>>>> >>>>>>>> > >>>>> solution seems fairly high in complexity to me. Here is my >>>>>>>> > >>>>> counter-proposal for >>>>>>>> > >>>>> what we might do: >>>>>>>> > >>>>> >>>>>>>> > >>>>> 1. Add a read() API that works in a new "manual mode" >>>>>>>> only >>>>>>>> > >>>>>> 2. Add an option on socket creation to create the >>>>>>>> socket in >>>>>>>> > >>>>>> "manual mode" - >>>>>>>> > >>>>>> >>>>>>>> > >>>>> i.e. no auto-polling for receive >>>>>>>> > >>>>> >>>>>>>> > >>>>>> 3. Add an API to enable automatic receive polling (i.e. >>>>>>>> to leave >>>>>>>> > >>>>>> "manual >>>>>>>> > >>>>>> >>>>>>>> > >>>>> mode"). NO API for switching back to manual mode, because >>>>>>>> this is >>>>>>>> > >>>>> unnecessary >>>>>>>> > >>>>> and a source of much complexity here. >>>>>>>> > >>>>> >>>>>>>> > >>>>> Separately we could also address two (IMHO) lower priority >>>>>>>> issues: >>>>>>>> > >>>>>> >>>>>>>> > >>>>> >>>>>>>> > >>>>> 4. Defer the setPaused callback until the receive queue >>>>>>>> is >>>>>>>> > flushed, >>>>>>>> > >>>>>> just >>>>>>>> > >>>>>> >>>>>>>> > >>>>> because this feels like the right thing to do. >>>>>>>> > >>>>> >>>>>>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll >>>>>>>> mode, to >>>>>>>> > avoid >>>>>>>> > >>>>>> the >>>>>>>> > >>>>>> >>>>>>>> > >>>>> over-eager reading problem. >>>>>>>> > >>>>> >>>>>>>> > >>>>> I think adding 1-3 would be lower complexity than what you >>>>>>>> have >>>>>>>> > here, >>>>>>>> > >>>>>> AFAICT >>>>>>>> > >>>>>> >>>>>>>> > >>>>> it would address all of your needs, and it doesn't actually >>>>>>>> change >>>>>>>> > any >>>>>>>> > >>>>> behavior >>>>>>>> > >>>>> of existing API uses in the wild (for better or worse). >>>>>>>> > >>>>> >>>>>>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible >>>>>>>> throttling >>>>>>>> > >>>>>> solution >>>>>>>> > >>>>>> >>>>>>>> > >>>>> than strictly tying read call frequency to the JS event >>>>>>>> loop. >>>>>>>> > >>>>> >>>>>>>> > >>>>> WDYT? >>>>>>>> > >>>>>> >>>>>>>> > >>>>> >>>>>>>> > >>>>> Or maybe instead of "manual mode" which is a bit confusing, >>>>>>>> we have >>>>>>>> > a >>>>>>>> > >>>>> "start >>>>>>>> > >>>>> paused" option. Then read() is, as you suggest, a way of >>>>>>>> reading >>>>>>>> > from >>>>>>>> > >>>>> the socket >>>>>>>> > >>>>> while paused. The main difference is that we can avoid >>>>>>>> trying to >>>>>>>> > >>>>> resolve the >>>>>>>> > >>>>> complexity of pausing at a very specific place in the >>>>>>>> stream, by >>>>>>>> > >>>>> making it >>>>>>>> > >>>>> possible to start the socket in a paused state. >>>>>>>> > >>>>> >>>>>>>> > >>>>> The trade-off is that you have to do manual reads until >>>>>>>> you're ready >>>>>>>> > >>>>> to initiate >>>>>>>> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >>>>>>>> > >>>>> >>>>>>>> > >>>>> >>>>>>>> > >>>>> https://codereview.chromium.org/494573002/ >>>>>>>> > >>>>> >>>>>>>> > >>>> >>>>>>>> > >>>> >>>>>>>> > >>> >>>>>>>> > >> >>>>>>>> > > >>>>>>>> >>>>>>>> > -- >>>>>>>> > You received this message because you are subscribed to the >>>>>>>> Google Groups >>>>>>>> "Chromium-reviews" group. >>>>>>>> > To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an >>>>>>>> email to chromium-reviews+unsubscribe@chromium.org. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> https://codereview.chromium.org/494573002/ >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh, and I forgot to respond to your second part. There's another CL I've written (dependent on this) that makes the JS context (upon request) send back an EventAck event back when it's processed the event. The TCPSocketEventDispatcher waits to receive one of these before issuing its next Read(). Talking to Ken, it makes sense to allow more than 1 outstanding OnReceive event before issuing another Read, but let it be a socket option so that the user can limit this. We've been having major backpressure problems with uProxy, so this is rather important. We end up with massive JS event queues full of OnReceive events that don't drain until we notice that another queue is rather full -- where we pause the socket. But that logic doesn't know how many pending OnReceive events there are, so it's often way too late. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Dec 14, 2015 at 4:46 PM, Lally Singh <lally@google.com> wrote: > Hi Renaud, > > Thank you for this -- any ideas on making this simpler help. I don't > know how to make that work across the TLS boundary. The TLS socket uses > the native Read() api, and the TCPSocketEventDispatcher sits atop of the > TLS socket, calling Read() on it to get decrypted data. That's why there > are buffering versions of both socket types (TCPClientSocket and > StreamSocket) there --- to support both plaintext and TLS sockets. > > Would it help (Renaud, Ken) if I added a write-up of the implementation > changes to the doc? > > > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Mon, Dec 14, 2015 at 2:06 PM, Renaud Paquay <rpaquay@chromium.org> > wrote: > >> Reading through this thread, it seems the main concern is the complexity >> of the current proposed CL vs the benefit of having a stronger setPaused >> guarantee. So, I am assuming that if the CL was dead simple, there would be >> no strong argument against changing the "setPaused" behavior, right? >> >> So, if one of the main concern is complexity of the current CL, have you >> considered making a more local change, i.e. only >> changing tcp_socket_event_dispatcher.cc? >> >> I have not looked at the code too closely lately, but it seems that you >> could >> >> - Add a single io_buffer "buffered_data" to ResumableTCPSocket >> - In TCPSocketEventDispatcher::ReadCallback >> - If the ResumableTCPSocket is paused, transfer the io_buffer from >> the callback to "ResumableTCPSocket::buffered_data" >> - Note you should be able to DCHECK here that this happens at >> most once, and that you don't assign buffered_data twice >> - If the socket is not paused, do the usual (post >> a SOCKETS_TCP_ON_RECEIVE message) >> - In TCPSocketEventDispatcher::StartRead >> - After the "paused()" check, >> - If ResumableTCPSocket::buffered_data is set, post >> a SOCKETS_TCP_ON_RECEIVE message and reset buffered_data to NULL. >> - Then do the usual thing (start an async read) >> >> This would really be only 3 (simple?) local changes (highlighted parts). >> I am not 100% sure this would work, as the state machine is becoming quite >> complex, but maybe it is worth a try. >> >> That said, unless I am missing something, both solutions rely on the fact >> that the callback from setPaused will be invoked after *all* pending >> onReceive events are invoked (this can happen if the JS client does not >> have the time to process onReceive events fast enough). Do we know this is >> guaranteed by the Extension binding mechanism? If not, then this change >> alone would not improve things to a meaningful extend, and the (future) CL >> addressing the backpressure issue would be required for this to be >> effective. >> >> If I am totally off track, apologies, and feel free to ignore this :) >> >> Renaud >> >> >> >> On Mon, Dec 14, 2015 at 9:38 AM, Lucas Dixon <ldixon@google.com> wrote: >> >>> Reading a set of bytes can be important for STARTTLS flows, especially >>> because I don't think, in general, you get a guarantee about what each >>> packet contains. Without precise control of how much gets read, you can't >>> be sure that you pause/read the right amount before encryption starts. >>> >>> On Mon, 14 Dec 2015 at 11:46 Lally Singh <lally@google.com> wrote: >>> >>>> I asked around the team for where we were having and seeing pain points: >>>> >>>> * We use setpaused() for flow control. Even with backpressure, we >>>> still have to stop the incoming flow sometimes to let our internal queues >>>> drain out. The additional slop of an additional onReceive after we've >>>> already determined that we want to stop the influx reduces our control over >>>> the flow rates. We have a lot of simultaneous connections, so this adds up. >>>> >>>> * An external group doing IMAP: >>>> https://github.com/MobileChromeApps/mobile-chrome-apps/issues/269#issuecommen... - >>>> their workaround counts incoming onReceive packets and times a transmit >>>> right after a pause to get the sequencing right. But there are a bunch of >>>> API users on that thread, it's tough to tell what'll work for all of them. >>>> >>>> * From a team member here: >>>> >>>>> Yeah, this was a pain for me - it took a while to figure out what I >>>>> was doing wrong and realize it was an issue with the API rather than my >>>>> code. Then to fix it, I had to write a new "prepareSecure" method for both >>>>> freedom-for-firefox (a no-op) and freedom-for-chrome (calls .pause), plus >>>>> the common API definition in freedomjs and call it 1 read prior to wanting >>>>> to secure the socket. >>>>> >>>>> In the case of XMPP starttls flow we were able to work around this >>>>> because XMPP works like this: >>>>> >>>>> 1. client sends init message to server in cleartext >>>>> 2. server sends response in cleartext >>>>> 3. client sends some more data in cleartext >>>>> 4. server replies with starttls command in cleartext >>>>> 5. client upgrades the socket to TCP >>>>> >>>>> Because this flow is predictable we can call prepareSecure (pause) >>>>> before step 3 as we know there will be one more read coming up, and then at >>>>> step 5 we can actually call .secure. But there may be other protocols >>>>> where this doesn't work - the user might call .pause yet if the server >>>>> doesn't reply with anything then .pause will never fulfill and they can >>>>> never call .secure >>>>> >>>> [ Note that this is all talking XMPP, which is nontrival to implement >>>> on both read() and onReceive ] >>>> >>>> And we've got 19 stars on one of the bugs on this (crbug.com/403076) >>>> and 23 on a duplicate (crbug.com/467677) -- but I couldn't tell you >>>> about how many of those are overlaps. >>>> >>>> In contrast, combining this setPause() change with the backpressure CL, >>>> and setting N=1 (although I'll amend that CL to allow N to be adjustable) >>>> gives you completely precise socket reads. You can pause a socket during >>>> an onReceive event handler and it'll take effect immediately. That's very >>>> useful if you want to read the next K bytes to determine what to do next , >>>> set your onReceive event handlers to different functions, depending on what >>>> value you got for the K bytes, and then unpause the socket. >>>> >>>> >>>> >>>> >>>> >>>> Cheers, >>>> $ ls >>>> >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>> Engineering | lally@google.com >>>> >>>> >>>> On Thu, Dec 10, 2015 at 2:00 PM, Ken Rockot <rockot@chromium.org> >>>> wrote: >>>> >>>>> >>>>> >>>>> On Thu, Dec 10, 2015 at 10:52 AM, Lally Singh <lally@google.com> >>>>> wrote: >>>>> >>>>>> #2 is correct. >>>>>> >>>>>> #1 works fine as long as you have a trivial STARTLS protocol. For >>>>>> XMPP, we'd need to speak both read() and onReceive for the XMPP stack. Two >>>>>> different APIs, but you can bridge them with a buffer. We've got some >>>>>> server-specific hacks to work right now. It's a nasty pain point for us. >>>>>> >>>>>> I don't know if node compatibility was ever a goal for this API -- >>>>>> there are a *lot* of handy libraries you can use in a chrome app, but it's >>>>>> tougher with the different pause semantics between the two apis. >>>>>> >>>>>> Anyways, I'm spent. If these aren't compelling, then I'll dump the >>>>>> CL and push the test infrastructure in here to the other backpressure CL (I >>>>>> wrote it for both). >>>>>> >>>>> >>>>> Sorry this is being drawn out so much. It's helpful to understand the >>>>> requirements here in more detail. I don't mean to be obtuse, I just want to >>>>> make sure we have the right API. If start-paused is still painful, it's not >>>>> a good option. Server-specific hacks sound painful. >>>>> >>>>> >>>>>> >>>>>> >>>>>> >>>>>> Cheers, >>>>>> $ ls >>>>>> >>>>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>> Engineering | lally@google.com >>>>>> >>>>>> >>>>>> On Tue, Dec 8, 2015 at 4:26 PM, Ken Rockot <rockot@chromium.org> >>>>>> wrote: >>>>>> >>>>>>> I'm happy to schedule a VC, but I've read the document, and we've >>>>>>> discussed alternative ways of addressing both the secure socket flow and >>>>>>> the back-pressure problem, neither of which seem to require another layer >>>>>>> of implementation-level buffering to be solved. To summarize the >>>>>>> conclusions I drew from that discussion: >>>>>>> >>>>>>> 1. If creating a socket in a paused state helps make secure >>>>>>> socket flow less hacky to implement (seems like it does?), then the details >>>>>>> of set-paused behavior seem largely inconsequential to me and its >>>>>>> "broken"ness is rather subjective. >>>>>>> 2. Back-pressure can be addressed efficiently with a per-socket >>>>>>> option to configure an upper bound on the size of the receive-event backlog. >>>>>>> >>>>>>> My assumptions around #1 may be invalid. Does managing TLS setup >>>>>>> with a manual read() API (prior to initiating TLS and unpausing) add more >>>>>>> complexity than I'm imagining? >>>>>>> >>>>>>> >>>>>>> On Tue, Dec 8, 2015 at 1:14 PM, Lucas Dixon <ldixon@google.com> >>>>>>> wrote: >>>>>>> >>>>>>>> I believe Lally shared the the proposed design >>>>>>>> <https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/> quite >>>>>>>> a while ago and it was discussed a bunch then. My understanding is that its purpose >>>>>>>> is to fix two bad things with the current API: set-paused is >>>>>>>> broken, which means that many secure socket flows need to be hacked/don't >>>>>>>> work, and there is no back-pressure, which means you have to implement a >>>>>>>> very inefficient version in JS. Having back-pressure and secure sockets >>>>>>>> behave properly seems good to me, and is certainly what I'd expect of a tcp >>>>>>>> socket library. I suspect there is some basic misunderstanding here, maybe >>>>>>>> a short VC would help? >>>>>>>> >>>>>>>> On Tue, 8 Dec 2015 at 15:48 <rockot@chromium.org> wrote: >>>>>>>> >>>>>>>>> On 2015/12/07 at 20:12:49, chromium-reviews wrote: >>>>>>>>> > In avoiding this, I think we'd just be making clients implement >>>>>>>>> the same >>>>>>>>> > buffer in their code. >>>>>>>>> >>>>>>>>> > Which is sad, because the problem is ours and we're doing a poor >>>>>>>>> job here. >>>>>>>>> > Node.js implements a similar pause API and does a better job >>>>>>>>> here. >>>>>>>>> >>>>>>>>> I would prefer our APIs remain minimalist in their design. >>>>>>>>> Auto-polling at >>>>>>>>> the >>>>>>>>> implementation layer adds value in that it avoids some latency >>>>>>>>> imposed by >>>>>>>>> the JS >>>>>>>>> event loop. I don't a similar level of value being added by this >>>>>>>>> buffering >>>>>>>>> behavior. >>>>>>>>> >>>>>>>>> Since pause can't actually stop a pending read from completing or >>>>>>>>> data from >>>>>>>>> arriving, this buffering only artificially hides an event from the >>>>>>>>> client. >>>>>>>>> Why >>>>>>>>> is that better than firing the event and letting the client decide >>>>>>>>> what to >>>>>>>>> do? >>>>>>>>> I'm certain that not every client is averse to receiving and >>>>>>>>> processing some >>>>>>>>> data after issuing a pause. >>>>>>>>> >>>>>>>>> >>>>>>>>> > This CL is a buffer, some plumbing, and test cases. If it's >>>>>>>>> too big or >>>>>>>>> > there are ways you can see to make it smaller, I'm happy to >>>>>>>>> break it up or >>>>>>>>> > adjust it. >>>>>>>>> >>>>>>>>> > We (uProxy) use the socket api quite a bit, and implement quite >>>>>>>>> a bit >>>>>>>>> > atop: >>>>>>>>> > a SOCKS5 server, an XHR client, and an XMPP client. We've been >>>>>>>>> hitting >>>>>>>>> > issues with sockets (basically, pausing and backpressure) for a >>>>>>>>> while, and >>>>>>>>> > I'm quite eager to fix them. The workarounds in code are quite >>>>>>>>> painful >>>>>>>>> > and >>>>>>>>> > cost us substantial amounts of memory and performance. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> > Cheers, >>>>>>>>> > $ ls >>>>>>>>> >>>>>>>>> > Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>>>>> Engineering | >>>>>>>>> > lally@google.com >>>>>>>>> >>>>>>>>> >>>>>>>>> > On Mon, Dec 7, 2015 at 12:17 PM, Ken Rockot <rockot@chromium.org> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> > > I'm not convinced it needs to be fixed as long as it's >>>>>>>>> understood that >>>>>>>>> > > there's no guarantee that data won't arrive after pause. I can >>>>>>>>> imagine >>>>>>>>> > > cases where it's still useful without that guarantee, but your >>>>>>>>> case >>>>>>>>> > isn't >>>>>>>>> > > one of them. >>>>>>>>> > > >>>>>>>>> > > On Mon, Dec 7, 2015 at 9:14 AM, Lally Singh <lally@google.com> >>>>>>>>> wrote: >>>>>>>>> > > >>>>>>>>> > >> I'm happy with an N > 1 approach. >>>>>>>>> > >> >>>>>>>>> > >> I'm not sure how else to fix the currently-broken setPaused() >>>>>>>>> api other >>>>>>>>> > >> than buffering. >>>>>>>>> > >> >>>>>>>>> > >> Cheers, >>>>>>>>> > >> $ ls >>>>>>>>> > >> >>>>>>>>> > >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>>>>> > Engineering | >>>>>>>>> > >> lally@google.com >>>>>>>>> > >> >>>>>>>>> > >> >>>>>>>>> > >> On Mon, Dec 7, 2015 at 11:32 AM, Ken Rockot < >>>>>>>>> rockot@chromium.org> >>>>>>>>> > wrote: >>>>>>>>> > >> >>>>>>>>> > >>> I think it would be reasonable (for #5) to have a max >>>>>>>>> pending receive >>>>>>>>> > >>> option. It would work generally as you describe (don't read >>>>>>>>> until >>>>>>>>> > event(s) >>>>>>>>> > >>> have been acked by JS), it would just support N > 1 >>>>>>>>> outstanding >>>>>>>>> > receive >>>>>>>>> > >>> events. >>>>>>>>> > >>> >>>>>>>>> > >>> Re #4: Doh... right, I see, because we can't just cancel an >>>>>>>>> > outstanding >>>>>>>>> > >>> read syscall. In that case I'd like to just continue on a >>>>>>>>> path that >>>>>>>>> doesn't >>>>>>>>> > >>> require us to solve that problem, which I think start-paused >>>>>>>>> does. :) >>>>>>>>> > >>> >>>>>>>>> > >>> On Mon, Dec 7, 2015 at 8:23 AM, Lally Singh < >>>>>>>>> lally@google.com> wrote: >>>>>>>>> > >>> >>>>>>>>> > >>>> I'm happy with a "start paused" option, and a read() method >>>>>>>>> to >>>>>>>>> > access a >>>>>>>>> > >>>> paused stream. I think that's a natural extension of the >>>>>>>>> current >>>>>>>>> > API. >>>>>>>>> > >>>> >>>>>>>>> > >>>> For #4: The problem with just deferring the setPaused() >>>>>>>>> callback is >>>>>>>>> > >>>> that data may not be coming back anytime soon, so that >>>>>>>>> callback can >>>>>>>>> > sit >>>>>>>>> and >>>>>>>>> > >>>> wait a long time. I'm not sure how to get around that >>>>>>>>> without >>>>>>>>> buffering. >>>>>>>>> > >>>> >>>>>>>>> > >>>> For #5: any thoughts on what kind of throttle specification >>>>>>>>> that'd >>>>>>>>> > be? >>>>>>>>> > >>>> A max-outstanding-JS events count, where you can specify a >>>>>>>>> count > >>>>>>>>> > 1, can >>>>>>>>> > >>>> keep throughput high. But if we stop talking in terms of >>>>>>>>> > outstanding JS >>>>>>>>> > >>>> events, we have to find another way to maintain sensitivity >>>>>>>>> to CPU >>>>>>>>> pressure. >>>>>>>>> > >>>> >>>>>>>>> > >>>> >>>>>>>>> > >>>> >>>>>>>>> > >>>> Cheers, >>>>>>>>> > >>>> $ ls >>>>>>>>> > >>>> >>>>>>>>> > >>>> Lally Singh | Google Ideas <http://www.google.com/ideas/> | >>>>>>>>> > >>>> Engineering | lally@google.com >>>>>>>>> > >>>> >>>>>>>>> > >>>> >>>>>>>>> > >>>> On Mon, Dec 7, 2015 at 11:00 AM, <rockot@chromium.org> >>>>>>>>> wrote: >>>>>>>>> > >>>> >>>>>>>>> > >>>>> On 2015/12/07 at 15:54:29, Ken Rockot wrote: >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> On 2015/12/04 at 19:32:40, lally wrote: >>>>>>>>> > >>>>>> > Hello, this is a fix to sockets.tcp's setPause() >>>>>>>>> implementation. >>>>>>>>> > >>>>>> setPause() >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> has a TCPSocketEventDispatcher, which issues Read()s on >>>>>>>>> the socket, >>>>>>>>> > >>>>> and then >>>>>>>>> > >>>>> issues onReceive events back to JS when those Read()s >>>>>>>>> complete. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> > >>>>>>>>> > >>>>>> > But, when pausing, any issued Read() continues to live >>>>>>>>> on. >>>>>>>>> > >>>>>> Currently, that >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> means that another onReceive() is likely to come, even >>>>>>>>> when the >>>>>>>>> > >>>>> socket's paused. >>>>>>>>> > >>>>> This change buffers the data that comes back from Read() >>>>>>>>> for >>>>>>>>> > sending >>>>>>>>> > >>>>> later, >>>>>>>>> > >>>>> when the socket is un-paused. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> > >>>>>>>>> > >>>>>> > I've written up the issues here: >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> >>>>>>>>> >>>>>>>>> https://docs.google.com/document/d/1sAWQZ0Dvbz-xl_zo6Qv4CARIBA5BLrb0SKPiao8sy3c/ >>>>>>>>> > >>>>> . >>>>>>>>> > >>>>> We don't do backpressure here (that's in a follow-up CL), >>>>>>>>> but some >>>>>>>>> > of >>>>>>>>> > >>>>> the >>>>>>>>> > >>>>> testing support needed for that is included here. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> > >>>>>>>>> > >>>>>> > >>>>>>>>> > >>>>>> > Please take a look, and let me know if you have any >>>>>>>>> questions. >>>>>>>>> > I'm >>>>>>>>> > >>>>>> happy to >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> talk about this. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> > >>>>>>>>> > >>>>>> > I spoke to rsleevi@ privately, and he's not interested >>>>>>>>> in >>>>>>>>> > >>>>>> reviewing this CL. >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> This is an unfortunate problem and I agree that we need to >>>>>>>>> fix it, >>>>>>>>> > but >>>>>>>>> > >>>>>> this >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> solution seems fairly high in complexity to me. Here is my >>>>>>>>> > >>>>> counter-proposal for >>>>>>>>> > >>>>> what we might do: >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> 1. Add a read() API that works in a new "manual mode" >>>>>>>>> only >>>>>>>>> > >>>>>> 2. Add an option on socket creation to create the >>>>>>>>> socket in >>>>>>>>> > >>>>>> "manual mode" - >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> i.e. no auto-polling for receive >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> 3. Add an API to enable automatic receive polling >>>>>>>>> (i.e. to leave >>>>>>>>> > >>>>>> "manual >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> mode"). NO API for switching back to manual mode, because >>>>>>>>> this is >>>>>>>>> > >>>>> unnecessary >>>>>>>>> > >>>>> and a source of much complexity here. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> Separately we could also address two (IMHO) lower priority >>>>>>>>> issues: >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> 4. Defer the setPaused callback until the receive queue >>>>>>>>> is >>>>>>>>> > flushed, >>>>>>>>> > >>>>>> just >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> because this feels like the right thing to do. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>>> 5. Add an API option to throttle reads in auto-poll >>>>>>>>> mode, to >>>>>>>>> > avoid >>>>>>>>> > >>>>>> the >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> over-eager reading problem. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> I think adding 1-3 would be lower complexity than what you >>>>>>>>> have >>>>>>>>> > here, >>>>>>>>> > >>>>>> AFAICT >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> it would address all of your needs, and it doesn't >>>>>>>>> actually change >>>>>>>>> > any >>>>>>>>> > >>>>> behavior >>>>>>>>> > >>>>> of existing API uses in the wild (for better or worse). >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> #4 is nice to have, #5 seems to me like a more flexible >>>>>>>>> throttling >>>>>>>>> > >>>>>> solution >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> than strictly tying read call frequency to the JS event >>>>>>>>> loop. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> WDYT? >>>>>>>>> > >>>>>> >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> Or maybe instead of "manual mode" which is a bit >>>>>>>>> confusing, we have >>>>>>>>> > a >>>>>>>>> > >>>>> "start >>>>>>>>> > >>>>> paused" option. Then read() is, as you suggest, a way of >>>>>>>>> reading >>>>>>>>> > from >>>>>>>>> > >>>>> the socket >>>>>>>>> > >>>>> while paused. The main difference is that we can avoid >>>>>>>>> trying to >>>>>>>>> > >>>>> resolve the >>>>>>>>> > >>>>> complexity of pausing at a very specific place in the >>>>>>>>> stream, by >>>>>>>>> > >>>>> making it >>>>>>>>> > >>>>> possible to start the socket in a paused state. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> The trade-off is that you have to do manual reads until >>>>>>>>> you're ready >>>>>>>>> > >>>>> to initiate >>>>>>>>> > >>>>> TLS and unpause, but that doesn't seem so bad to me. >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> >>>>>>>>> > >>>>> https://codereview.chromium.org/494573002/ >>>>>>>>> > >>>>> >>>>>>>>> > >>>> >>>>>>>>> > >>>> >>>>>>>>> > >>> >>>>>>>>> > >> >>>>>>>>> > > >>>>>>>>> >>>>>>>>> > -- >>>>>>>>> > You received this message because you are subscribed to the >>>>>>>>> Google Groups >>>>>>>>> "Chromium-reviews" group. >>>>>>>>> > To unsubscribe from this group and stop receiving emails from >>>>>>>>> it, send an >>>>>>>>> email to chromium-reviews+unsubscribe@chromium.org. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> https://codereview.chromium.org/494573002/ >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I did a first pass here. Note that I haven't reviewed the details of SocketPauseBuffer's implementation at all yet, and this review mostly covers stylistic and readability concerns. But before reading all my inline comments... I really, really want to make sure we're doing the right thing here and I'm still not convinced :( It seems very sad to say we're going to only allow 1 pending receive event at a time. I know you agreed that we can/should add an option for variable queue length, but that's only useful for configuring back-pressure when NOT using TLS. AFAICT (please correct me if I'm wrong) many folks who need to perform TLS setup (including uProxy) will in practice be forced to set the queue length to 1, because you need to ensure that every other receive is still buffered in the browser process at the time you call secure(). This means that in practice you're going to force a complete IPC round-trip between every socket read, and that can't be good. Of course you could bump up the receive queue length once TLS is established, but what do you do on a connection that isn't TLS yet but which might want to upgrade at any time? Here's another strawman counter-proposal: 1. Implement a max receive queue option *strictly* to support efficient back-pressure. 2. Add a private API that lets internal bindings push data from pending receive events back the browser 3. Implement additional pause logic in the JS layer that immediately stops receive events from firing 4. (optional if needed) implement a read() API which can be used while paused. This can pull data from buffered events if available. 5. have secure() push any pending receive event data back to the browser before upgrading This of course introduces some non-trivial complexity to support pushing data back into socket objects, and that might make it undesirable... but it allows you to reliably implement back-pressure and get the setPaused() and secure() semantics you desire without forcing you to settle for higher-latency reads everywhere. WDYT? https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... File extensions/browser/api/socket/passthrough.h (right): https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/passthrough.h:24: class Passthrough : public DefaultInitializer<BaseSocket> { I am generally not happy with this approach. It saves a few lines of code at the expense of readability. Could you please just delete Passthrough<T> and DefaultInitializer<T>, and and implement BufferingStreamSocket and BufferingTCPClientSocket without them? There should not be much redundancy since BufferingTCPClientSocket can still inherit from BufferingStreamSocket. https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/passthrough.h:27: : base_socket_(impl.Pass()) {} nit: (everywhere) New code should use std::move instead of Pass() https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... File extensions/browser/api/socket/tcp_socket.h (right): https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:23: typedef base::Callback<void(void)> PauseCompletionCallback; nit: please use a type alias instead of typedef now https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:33: class SocketPauseBuffer { This class should be in its own header and cc file https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:35: typedef base::Callback< nit: type alias https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:89: void InsureBufferCanHold(int num_bytes); nit: s/Insure/Ensure/ https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:103: // BufferingStreamSocket is a client of downstream_socket_. downstream_socket_ doesn't exist https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:105: // scoped_ptr<net::StreamSocket> downstream_socket_; Remove this? https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:144: class BufferingStreamSocket : public Passthrough<net::StreamSocket> { This class should have its own header and cc https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:174: class BufferingTCPClientSocket : public Passthrough<net::TCPClientSocket> { This class should have its own header and cc https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tcp_socket.h:314: void ApiPauseComplete(); nit: How about NotifyPauseComplete? https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tls_socket.cc:53: // Wrap the StreamSocket in a BufferingTCPSocket, to support SetPaused(). nit: s/BufferingTCPSocket/BufferingStreamSocket/ https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/socket/tls_socket.cc:54: scoped_ptr<BufferingStreamSocket> buffer_sock( nit: buffer_socket? No real value in abbreviating here https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:5: #include <hash_set> Thanks for adding this :) But it should be "base/containers/hash_tables.h" (and down below with chromium headers) https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:198: client_sock->Pause(); How about just making Pause take a callback? Or if that's too annoying (i.e. too many existing callsites to update), how about introducing a PauseAsync call which takes a callback?
Thanks Ken. I'm pretty happy to look into this. Thank you for your consideration. I'll ping back when I've got an idea of what's involved, to make sure you're OK with that. I'll also dump the Passthrough :-) Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Tue, Dec 15, 2015 at 12:17 PM, <rockot@chromium.org> wrote: > I did a first pass here. Note that I haven't reviewed the details of > SocketPauseBuffer's implementation at all yet, and this review mostly > covers > stylistic and readability concerns. But before reading all my inline > comments... > > I really, really want to make sure we're doing the right thing here and I'm > still not convinced :( > > It seems very sad to say we're going to only allow 1 pending receive event > at a > time. I know you agreed that we can/should add an option for variable queue > length, but that's only useful for configuring back-pressure when NOT > using TLS. > > AFAICT (please correct me if I'm wrong) many folks who need to perform TLS > setup > (including uProxy) will in practice be forced to set the queue length to 1, > because you need to ensure that every other receive is still buffered in > the > browser process at the time you call secure(). > > This means that in practice you're going to force a complete IPC round-trip > between every socket read, and that can't be good. Of course you could > bump up > the receive queue length once TLS is established, but what do you do on a > connection that isn't TLS yet but which might want to upgrade at any time? > > Here's another strawman counter-proposal: > 1. Implement a max receive queue option *strictly* to support efficient > back-pressure. > 2. Add a private API that lets internal bindings push data from pending > receive > events back the browser > 3. Implement additional pause logic in the JS layer that immediately stops > receive events from firing > 4. (optional if needed) implement a read() API which can be used while > paused. > This can pull data from buffered events if available. > 5. have secure() push any pending receive event data back to the browser > before > upgrading > > This of course introduces some non-trivial complexity to support pushing > data > back into socket objects, and that might make it undesirable... but it > allows > you to reliably implement back-pressure and get the setPaused() and > secure() > semantics you desire without forcing you to settle for higher-latency reads > everywhere. WDYT? > > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/socket/passthrough.h (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/passthrough.h:24: class Passthrough : > public DefaultInitializer<BaseSocket> { > I am generally not happy with this approach. It saves a few lines of > code at the expense of readability. > > Could you please just delete Passthrough<T> and DefaultInitializer<T>, > and and implement BufferingStreamSocket and BufferingTCPClientSocket > without them? > > There should not be much redundancy since BufferingTCPClientSocket can > still inherit from BufferingStreamSocket. > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/passthrough.h:27: : > base_socket_(impl.Pass()) {} > nit: (everywhere) New code should use std::move instead of Pass() > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/socket/tcp_socket.h (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:23: typedef > base::Callback<void(void)> PauseCompletionCallback; > nit: please use a type alias instead of typedef now > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:33: class SocketPauseBuffer { > This class should be in its own header and cc file > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:35: typedef base::Callback< > nit: type alias > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:89: void > InsureBufferCanHold(int num_bytes); > nit: s/Insure/Ensure/ > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:103: // BufferingStreamSocket > is a client of downstream_socket_. > downstream_socket_ doesn't exist > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:105: // > scoped_ptr<net::StreamSocket> downstream_socket_; > Remove this? > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:144: class > BufferingStreamSocket : public Passthrough<net::StreamSocket> { > This class should have its own header and cc > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:174: class > BufferingTCPClientSocket : public Passthrough<net::TCPClientSocket> { > This class should have its own header and cc > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:314: void ApiPauseComplete(); > nit: How about NotifyPauseComplete? > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/socket/tls_socket.cc (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tls_socket.cc:53: // Wrap the StreamSocket > in a BufferingTCPSocket, to support SetPaused(). > nit: s/BufferingTCPSocket/BufferingStreamSocket/ > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tls_socket.cc:54: > scoped_ptr<BufferingStreamSocket> buffer_sock( > nit: buffer_socket? No real value in abbreviating here > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:5: #include > <hash_set> > Thanks for adding this :) But it should be > "base/containers/hash_tables.h" (and down below with chromium headers) > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:198: > client_sock->Pause(); > How about just making Pause take a callback? Or if that's too annoying > (i.e. too many existing callsites to update), how about introducing a > PauseAsync call which takes a callback? > > https://codereview.chromium.org/494573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Ken, I've been thinking about this. How's this? 1. The TCPSocketEventDispatcher keeps buffers for the last N reads, feeding them once at a time to JavaScript. 2. If JavaScript pauses the socket, it pauses similarly to the current CL (modulo any changes you've wanted on it), and pushes the data from the buffers back into the SocketBuffer we have now (conceptually -- there are some optimizations available here) 3. If we secure(), the pushed-back data naturally gets fed into the TLS stack. 4. On unpause, the TCPSocketEventDispatcher will re-execute N reads from the socket, putting them in buffers to dispatch to JavaScript. If data in the buffers from step 1 were encrypted, these N buffers will be unencrypted versions (roughly -- handshakes, headers, etc., change the size relationship between cyphertext and cleartext) of them. The big benefit is that we don't have to try to un-queue javascript events, which avoids trying to push data backwards from the javascript-running thread back to the TCPSocketEventDispatcher's thread. So far, AFAICT, there isn't any per-event specialization that fiddles with that queue, so it seems avoiding special logic for onReceive is preferable. We can be a bit smart about when we generate the actual JS events and keep data in the SocketBuffer until absolutely necessary, making the push-back just a subtraction or two. But you still have two cons on read latency: - we still read into a separate buffer and copy into events - still a full round-trip per read event, but that's only an issue if there's another read event waiting for you when the ack comes back. High-throughput apps should probably raise the per-read size (independent of this change), and possibly reduce N if they don't want excess RAM overhead. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Tue, Dec 15, 2015 at 12:17 PM, <rockot@chromium.org> wrote: > I did a first pass here. Note that I haven't reviewed the details of > SocketPauseBuffer's implementation at all yet, and this review mostly > covers > stylistic and readability concerns. But before reading all my inline > comments... > > I really, really want to make sure we're doing the right thing here and I'm > still not convinced :( > > It seems very sad to say we're going to only allow 1 pending receive event > at a > time. I know you agreed that we can/should add an option for variable queue > length, but that's only useful for configuring back-pressure when NOT > using TLS. > > AFAICT (please correct me if I'm wrong) many folks who need to perform TLS > setup > (including uProxy) will in practice be forced to set the queue length to 1, > because you need to ensure that every other receive is still buffered in > the > browser process at the time you call secure(). > > This means that in practice you're going to force a complete IPC round-trip > between every socket read, and that can't be good. Of course you could > bump up > the receive queue length once TLS is established, but what do you do on a > connection that isn't TLS yet but which might want to upgrade at any time? > > Here's another strawman counter-proposal: > 1. Implement a max receive queue option *strictly* to support efficient > back-pressure. > 2. Add a private API that lets internal bindings push data from pending > receive > events back the browser > 3. Implement additional pause logic in the JS layer that immediately stops > receive events from firing > 4. (optional if needed) implement a read() API which can be used while > paused. > This can pull data from buffered events if available. > 5. have secure() push any pending receive event data back to the browser > before > upgrading > > This of course introduces some non-trivial complexity to support pushing > data > back into socket objects, and that might make it undesirable... but it > allows > you to reliably implement back-pressure and get the setPaused() and > secure() > semantics you desire without forcing you to settle for higher-latency reads > everywhere. WDYT? > > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/socket/passthrough.h (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/passthrough.h:24: class Passthrough : > public DefaultInitializer<BaseSocket> { > I am generally not happy with this approach. It saves a few lines of > code at the expense of readability. > > Could you please just delete Passthrough<T> and DefaultInitializer<T>, > and and implement BufferingStreamSocket and BufferingTCPClientSocket > without them? > > There should not be much redundancy since BufferingTCPClientSocket can > still inherit from BufferingStreamSocket. > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/passthrough.h:27: : > base_socket_(impl.Pass()) {} > nit: (everywhere) New code should use std::move instead of Pass() > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/socket/tcp_socket.h (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:23: typedef > base::Callback<void(void)> PauseCompletionCallback; > nit: please use a type alias instead of typedef now > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:33: class SocketPauseBuffer { > This class should be in its own header and cc file > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:35: typedef base::Callback< > nit: type alias > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:89: void > InsureBufferCanHold(int num_bytes); > nit: s/Insure/Ensure/ > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:103: // BufferingStreamSocket > is a client of downstream_socket_. > downstream_socket_ doesn't exist > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:105: // > scoped_ptr<net::StreamSocket> downstream_socket_; > Remove this? > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:144: class > BufferingStreamSocket : public Passthrough<net::StreamSocket> { > This class should have its own header and cc > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:174: class > BufferingTCPClientSocket : public Passthrough<net::TCPClientSocket> { > This class should have its own header and cc > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tcp_socket.h:314: void ApiPauseComplete(); > nit: How about NotifyPauseComplete? > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/socket/tls_socket.cc (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tls_socket.cc:53: // Wrap the StreamSocket > in a BufferingTCPSocket, to support SetPaused(). > nit: s/BufferingTCPSocket/BufferingStreamSocket/ > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/socket/tls_socket.cc:54: > scoped_ptr<BufferingStreamSocket> buffer_sock( > nit: buffer_socket? No real value in abbreviating here > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:5: #include > <hash_set> > Thanks for adding this :) But it should be > "base/containers/hash_tables.h" (and down below with chromium headers) > > > https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... > extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:198: > client_sock->Pause(); > How about just making Pause take a callback? Or if that's too annoying > (i.e. too many existing callsites to update), how about introducing a > PauseAsync call which takes a callback? > > https://codereview.chromium.org/494573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Dec 22, 2015 at 11:44 AM, Lally Singh <lally@google.com> wrote: > Hi Ken, > > I've been thinking about this. How's this? > > 1. The TCPSocketEventDispatcher keeps buffers for the last N reads, > feeding them once at a time to JavaScript. > I don't really see the point of buffering in the event dispatcher. That introduces roughly the same complexity of my proposal (i.e. having to push data back to buffers) without avoiding unnecessary receive latency. If we're going to dismiss the importance of avoiding said latency, IMHO we should just go with your original approach and buffer at the socket API layer. But I'm not convinced that we should dismiss it. > 2. If JavaScript pauses the socket, it pauses similarly to the current CL > (modulo any changes you've wanted on it), and pushes the data from the > buffers back into the SocketBuffer we have now (conceptually -- there are > some optimizations available here) > 3. If we secure(), the pushed-back data naturally gets fed into the TLS > stack. > 4. On unpause, the TCPSocketEventDispatcher will re-execute N reads from > the socket, putting them in buffers to dispatch to JavaScript. If data in > the buffers from step 1 were encrypted, these N buffers will be unencrypted > versions (roughly -- handshakes, headers, etc., change the size > relationship between cyphertext and cleartext) of them. > > The big benefit is that we don't have to try to un-queue javascript > events, which avoids trying to push data backwards from the > javascript-running thread back to the TCPSocketEventDispatcher's thread. > So far, AFAICT, there isn't any per-event specialization that fiddles with > that queue, so it seems avoiding special logic for onReceive is > preferable. We can be a bit smart about when we generate the actual JS > events and keep data in the SocketBuffer until absolutely necessary, making > the push-back just a subtraction or two. > No need to muck with generic event dispatch logic. The idea would be to introduce a new event (e.g. onReceiveInternal) which TCPSocketEventDispatcher would dispatch instead of onReceive. We'd add a listener for this event in custom bindings JS which would do its own simple queuing, and would dispatch onReceive events as desired. The custom bindings could then trivially implement a synchronous pause() entirely in JS. secure() would be straightforward as well: we'd move the existing secure() to a secureInternal() (still a browser-side C++ impl) which also takes a list of outstanding receive buffers. The JS implementation of secure() would steal all queued receives and feed them to secureInternal(). > But you still have two cons on read latency: > - we still read into a separate buffer and copy into events > - still a full round-trip per read event, but that's only an issue if > there's another read event waiting for you when the ack comes back. > High-throughput apps should probably raise the per-read size (independent > of this change), and possibly reduce N if they don't want excess RAM > overhead. > > > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > On Tue, Dec 15, 2015 at 12:17 PM, <rockot@chromium.org> wrote: > >> I did a first pass here. Note that I haven't reviewed the details of >> SocketPauseBuffer's implementation at all yet, and this review mostly >> covers >> stylistic and readability concerns. But before reading all my inline >> comments... >> >> I really, really want to make sure we're doing the right thing here and >> I'm >> still not convinced :( >> >> It seems very sad to say we're going to only allow 1 pending receive >> event at a >> time. I know you agreed that we can/should add an option for variable >> queue >> length, but that's only useful for configuring back-pressure when NOT >> using TLS. >> >> AFAICT (please correct me if I'm wrong) many folks who need to perform >> TLS setup >> (including uProxy) will in practice be forced to set the queue length to >> 1, >> because you need to ensure that every other receive is still buffered in >> the >> browser process at the time you call secure(). >> >> This means that in practice you're going to force a complete IPC >> round-trip >> between every socket read, and that can't be good. Of course you could >> bump up >> the receive queue length once TLS is established, but what do you do on a >> connection that isn't TLS yet but which might want to upgrade at any time? >> >> Here's another strawman counter-proposal: >> 1. Implement a max receive queue option *strictly* to support efficient >> back-pressure. >> 2. Add a private API that lets internal bindings push data from pending >> receive >> events back the browser >> 3. Implement additional pause logic in the JS layer that immediately >> stops >> receive events from firing >> 4. (optional if needed) implement a read() API which can be used while >> paused. >> This can pull data from buffered events if available. >> 5. have secure() push any pending receive event data back to the browser >> before >> upgrading >> >> This of course introduces some non-trivial complexity to support pushing >> data >> back into socket objects, and that might make it undesirable... but it >> allows >> you to reliably implement back-pressure and get the setPaused() and >> secure() >> semantics you desire without forcing you to settle for higher-latency >> reads >> everywhere. WDYT? >> >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> File extensions/browser/api/socket/passthrough.h (right): >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/passthrough.h:24: class Passthrough : >> public DefaultInitializer<BaseSocket> { >> I am generally not happy with this approach. It saves a few lines of >> code at the expense of readability. >> >> Could you please just delete Passthrough<T> and DefaultInitializer<T>, >> and and implement BufferingStreamSocket and BufferingTCPClientSocket >> without them? >> >> There should not be much redundancy since BufferingTCPClientSocket can >> still inherit from BufferingStreamSocket. >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/passthrough.h:27: : >> base_socket_(impl.Pass()) {} >> nit: (everywhere) New code should use std::move instead of Pass() >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> File extensions/browser/api/socket/tcp_socket.h (right): >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:23: typedef >> base::Callback<void(void)> PauseCompletionCallback; >> nit: please use a type alias instead of typedef now >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:33: class SocketPauseBuffer { >> This class should be in its own header and cc file >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:35: typedef base::Callback< >> nit: type alias >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:89: void >> InsureBufferCanHold(int num_bytes); >> nit: s/Insure/Ensure/ >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:103: // BufferingStreamSocket >> is a client of downstream_socket_. >> downstream_socket_ doesn't exist >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:105: // >> scoped_ptr<net::StreamSocket> downstream_socket_; >> Remove this? >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:144: class >> BufferingStreamSocket : public Passthrough<net::StreamSocket> { >> This class should have its own header and cc >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:174: class >> BufferingTCPClientSocket : public Passthrough<net::TCPClientSocket> { >> This class should have its own header and cc >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tcp_socket.h:314: void ApiPauseComplete(); >> nit: How about NotifyPauseComplete? >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> File extensions/browser/api/socket/tls_socket.cc (right): >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tls_socket.cc:53: // Wrap the StreamSocket >> in a BufferingTCPSocket, to support SetPaused(). >> nit: s/BufferingTCPSocket/BufferingStreamSocket/ >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/socket/tls_socket.cc:54: >> scoped_ptr<BufferingStreamSocket> buffer_sock( >> nit: buffer_socket? No real value in abbreviating here >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:5: #include >> <hash_set> >> Thanks for adding this :) But it should be >> "base/containers/hash_tables.h" (and down below with chromium headers) >> >> >> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >> extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:198: >> client_sock->Pause(); >> How about just making Pause take a callback? Or if that's too annoying >> (i.e. too many existing callsites to update), how about introducing a >> PauseAsync call which takes a callback? >> >> https://codereview.chromium.org/494573002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, Jan 4, 2016 at 11:39 AM, Ken Rockot <rockot@chromium.org> wrote: > On Tue, Dec 22, 2015 at 11:44 AM, Lally Singh <lally@google.com> wrote: > >> Hi Ken, >> >> I've been thinking about this. How's this? >> >> 1. The TCPSocketEventDispatcher keeps buffers for the last N reads, >> feeding them once at a time to JavaScript. >> > > I don't really see the point of buffering in the event dispatcher. That > introduces roughly the same complexity of my proposal (i.e. having to push > data back to buffers) without avoiding unnecessary receive latency. > > If we're going to dismiss the importance of avoiding said latency, IMHO we > should just go with your original approach and buffer at the socket API > layer. But I'm not convinced that we should dismiss it. > > >> 2. If JavaScript pauses the socket, it pauses similarly to the current CL >> (modulo any changes you've wanted on it), and pushes the data from the >> buffers back into the SocketBuffer we have now (conceptually -- there are >> some optimizations available here) >> 3. If we secure(), the pushed-back data naturally gets fed into the TLS >> stack. >> 4. On unpause, the TCPSocketEventDispatcher will re-execute N reads from >> the socket, putting them in buffers to dispatch to JavaScript. If data in >> the buffers from step 1 were encrypted, these N buffers will be unencrypted >> versions (roughly -- handshakes, headers, etc., change the size >> relationship between cyphertext and cleartext) of them. >> >> The big benefit is that we don't have to try to un-queue javascript >> events, which avoids trying to push data backwards from the >> javascript-running thread back to the TCPSocketEventDispatcher's thread. >> So far, AFAICT, there isn't any per-event specialization that fiddles with >> that queue, so it seems avoiding special logic for onReceive is >> preferable. We can be a bit smart about when we generate the actual JS >> events and keep data in the SocketBuffer until absolutely necessary, making >> the push-back just a subtraction or two. >> > > No need to muck with generic event dispatch logic. The idea would be to > introduce a new event (e.g. onReceiveInternal) which > TCPSocketEventDispatcher would dispatch instead of onReceive. We'd add a > listener for this event in custom bindings JS which would do its own simple > queuing, and would dispatch onReceive events as desired. > > The custom bindings could then trivially implement a synchronous pause() > entirely in JS. secure() would be straightforward as well: we'd move the > existing secure() to a secureInternal() (still a browser-side C++ impl) > which also takes a list of outstanding receive buffers. The JS > implementation of secure() would steal all queued receives and feed them to > secureInternal(). > Just another thought in addition: In order for the custom bindings approach to work well, we'd also need a JS implementation of close() to avoid leaking per-socket pause/queue state. > >> But you still have two cons on read latency: >> - we still read into a separate buffer and copy into events >> - still a full round-trip per read event, but that's only an issue if >> there's another read event waiting for you when the ack comes back. >> High-throughput apps should probably raise the per-read size (independent >> of this change), and possibly reduce N if they don't want excess RAM >> overhead. >> > >> >> >> >> Cheers, >> $ ls >> >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | >> lally@google.com >> >> >> On Tue, Dec 15, 2015 at 12:17 PM, <rockot@chromium.org> wrote: >> >>> I did a first pass here. Note that I haven't reviewed the details of >>> SocketPauseBuffer's implementation at all yet, and this review mostly >>> covers >>> stylistic and readability concerns. But before reading all my inline >>> comments... >>> >>> I really, really want to make sure we're doing the right thing here and >>> I'm >>> still not convinced :( >>> >>> It seems very sad to say we're going to only allow 1 pending receive >>> event at a >>> time. I know you agreed that we can/should add an option for variable >>> queue >>> length, but that's only useful for configuring back-pressure when NOT >>> using TLS. >>> >>> AFAICT (please correct me if I'm wrong) many folks who need to perform >>> TLS setup >>> (including uProxy) will in practice be forced to set the queue length to >>> 1, >>> because you need to ensure that every other receive is still buffered in >>> the >>> browser process at the time you call secure(). >>> >>> This means that in practice you're going to force a complete IPC >>> round-trip >>> between every socket read, and that can't be good. Of course you could >>> bump up >>> the receive queue length once TLS is established, but what do you do on a >>> connection that isn't TLS yet but which might want to upgrade at any >>> time? >>> >>> Here's another strawman counter-proposal: >>> 1. Implement a max receive queue option *strictly* to support efficient >>> back-pressure. >>> 2. Add a private API that lets internal bindings push data from pending >>> receive >>> events back the browser >>> 3. Implement additional pause logic in the JS layer that immediately >>> stops >>> receive events from firing >>> 4. (optional if needed) implement a read() API which can be used while >>> paused. >>> This can pull data from buffered events if available. >>> 5. have secure() push any pending receive event data back to the >>> browser before >>> upgrading >>> >>> This of course introduces some non-trivial complexity to support pushing >>> data >>> back into socket objects, and that might make it undesirable... but it >>> allows >>> you to reliably implement back-pressure and get the setPaused() and >>> secure() >>> semantics you desire without forcing you to settle for higher-latency >>> reads >>> everywhere. WDYT? >>> >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/socket/passthrough.h (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/passthrough.h:24: class Passthrough : >>> public DefaultInitializer<BaseSocket> { >>> I am generally not happy with this approach. It saves a few lines of >>> code at the expense of readability. >>> >>> Could you please just delete Passthrough<T> and DefaultInitializer<T>, >>> and and implement BufferingStreamSocket and BufferingTCPClientSocket >>> without them? >>> >>> There should not be much redundancy since BufferingTCPClientSocket can >>> still inherit from BufferingStreamSocket. >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/passthrough.h:27: : >>> base_socket_(impl.Pass()) {} >>> nit: (everywhere) New code should use std::move instead of Pass() >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/socket/tcp_socket.h (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:23: typedef >>> base::Callback<void(void)> PauseCompletionCallback; >>> nit: please use a type alias instead of typedef now >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:33: class SocketPauseBuffer { >>> This class should be in its own header and cc file >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:35: typedef base::Callback< >>> nit: type alias >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:89: void >>> InsureBufferCanHold(int num_bytes); >>> nit: s/Insure/Ensure/ >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:103: // BufferingStreamSocket >>> is a client of downstream_socket_. >>> downstream_socket_ doesn't exist >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:105: // >>> scoped_ptr<net::StreamSocket> downstream_socket_; >>> Remove this? >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:144: class >>> BufferingStreamSocket : public Passthrough<net::StreamSocket> { >>> This class should have its own header and cc >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:174: class >>> BufferingTCPClientSocket : public Passthrough<net::TCPClientSocket> { >>> This class should have its own header and cc >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:314: void ApiPauseComplete(); >>> nit: How about NotifyPauseComplete? >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/socket/tls_socket.cc (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tls_socket.cc:53: // Wrap the StreamSocket >>> in a BufferingTCPSocket, to support SetPaused(). >>> nit: s/BufferingTCPSocket/BufferingStreamSocket/ >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tls_socket.cc:54: >>> scoped_ptr<BufferingStreamSocket> buffer_sock( >>> nit: buffer_socket? No real value in abbreviating here >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:5: #include >>> <hash_set> >>> Thanks for adding this :) But it should be >>> "base/containers/hash_tables.h" (and down below with chromium headers) >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:198: >>> client_sock->Pause(); >>> How about just making Pause take a callback? Or if that's too annoying >>> (i.e. too many existing callsites to update), how about introducing a >>> PauseAsync call which takes a callback? >>> >>> https://codereview.chromium.org/494573002/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, Jan 4, 2016 at 2:39 PM, Ken Rockot <rockot@chromium.org> wrote: > On Tue, Dec 22, 2015 at 11:44 AM, Lally Singh <lally@google.com> wrote: > >> Hi Ken, >> >> I've been thinking about this. How's this? >> >> 1. The TCPSocketEventDispatcher keeps buffers for the last N reads, >> feeding them once at a time to JavaScript. >> > > I don't really see the point of buffering in the event dispatcher. That > introduces roughly the same complexity of my proposal (i.e. having to push > data back to buffers) without avoiding unnecessary receive latency. > > If we're going to dismiss the importance of avoiding said latency, IMHO we > should just go with your original approach and buffer at the socket API > layer. But I'm not convinced that we should dismiss it. > I didn't meant to dismiss latency. I just wanted to avoid touching the javascript event queue -- it's a separate part of code that doesn't have anyone else touching it in any way close to this right now. > > >> 2. If JavaScript pauses the socket, it pauses similarly to the current CL >> (modulo any changes you've wanted on it), and pushes the data from the >> buffers back into the SocketBuffer we have now (conceptually -- there are >> some optimizations available here) >> 3. If we secure(), the pushed-back data naturally gets fed into the TLS >> stack. >> 4. On unpause, the TCPSocketEventDispatcher will re-execute N reads from >> the socket, putting them in buffers to dispatch to JavaScript. If data in >> the buffers from step 1 were encrypted, these N buffers will be unencrypted >> versions (roughly -- handshakes, headers, etc., change the size >> relationship between cyphertext and cleartext) of them. >> >> The big benefit is that we don't have to try to un-queue javascript >> events, which avoids trying to push data backwards from the >> javascript-running thread back to the TCPSocketEventDispatcher's thread. >> So far, AFAICT, there isn't any per-event specialization that fiddles with >> that queue, so it seems avoiding special logic for onReceive is >> preferable. We can be a bit smart about when we generate the actual JS >> events and keep data in the SocketBuffer until absolutely necessary, making >> the push-back just a subtraction or two. >> > > No need to muck with generic event dispatch logic. The idea would be to > introduce a new event (e.g. onReceiveInternal) which > TCPSocketEventDispatcher would dispatch instead of onReceive. We'd add a > listener for this event in custom bindings JS which would do its own simple > queuing, and would dispatch onReceive events as desired. > > The custom bindings could then trivially implement a synchronous pause() > entirely in JS. secure() would be straightforward as well: we'd move the > existing secure() to a secureInternal() (still a browser-side C++ impl) > which also takes a list of outstanding receive buffers. The JS > implementation of secure() would steal all queued receives and feed them to > secureInternal(). > Oh, this all sounds pretty reasonable. > > >> But you still have two cons on read latency: >> - we still read into a separate buffer and copy into events >> - still a full round-trip per read event, but that's only an issue if >> there's another read event waiting for you when the ack comes back. >> High-throughput apps should probably raise the per-read size (independent >> of this change), and possibly reduce N if they don't want excess RAM >> overhead. >> > >> >> >> >> Cheers, >> $ ls >> >> Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | >> lally@google.com >> >> >> On Tue, Dec 15, 2015 at 12:17 PM, <rockot@chromium.org> wrote: >> >>> I did a first pass here. Note that I haven't reviewed the details of >>> SocketPauseBuffer's implementation at all yet, and this review mostly >>> covers >>> stylistic and readability concerns. But before reading all my inline >>> comments... >>> >>> I really, really want to make sure we're doing the right thing here and >>> I'm >>> still not convinced :( >>> >>> It seems very sad to say we're going to only allow 1 pending receive >>> event at a >>> time. I know you agreed that we can/should add an option for variable >>> queue >>> length, but that's only useful for configuring back-pressure when NOT >>> using TLS. >>> >>> AFAICT (please correct me if I'm wrong) many folks who need to perform >>> TLS setup >>> (including uProxy) will in practice be forced to set the queue length to >>> 1, >>> because you need to ensure that every other receive is still buffered in >>> the >>> browser process at the time you call secure(). >>> >>> This means that in practice you're going to force a complete IPC >>> round-trip >>> between every socket read, and that can't be good. Of course you could >>> bump up >>> the receive queue length once TLS is established, but what do you do on a >>> connection that isn't TLS yet but which might want to upgrade at any >>> time? >>> >>> Here's another strawman counter-proposal: >>> 1. Implement a max receive queue option *strictly* to support efficient >>> back-pressure. >>> 2. Add a private API that lets internal bindings push data from pending >>> receive >>> events back the browser >>> 3. Implement additional pause logic in the JS layer that immediately >>> stops >>> receive events from firing >>> 4. (optional if needed) implement a read() API which can be used while >>> paused. >>> This can pull data from buffered events if available. >>> 5. have secure() push any pending receive event data back to the >>> browser before >>> upgrading >>> >>> This of course introduces some non-trivial complexity to support pushing >>> data >>> back into socket objects, and that might make it undesirable... but it >>> allows >>> you to reliably implement back-pressure and get the setPaused() and >>> secure() >>> semantics you desire without forcing you to settle for higher-latency >>> reads >>> everywhere. WDYT? >>> >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/socket/passthrough.h (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/passthrough.h:24: class Passthrough : >>> public DefaultInitializer<BaseSocket> { >>> I am generally not happy with this approach. It saves a few lines of >>> code at the expense of readability. >>> >>> Could you please just delete Passthrough<T> and DefaultInitializer<T>, >>> and and implement BufferingStreamSocket and BufferingTCPClientSocket >>> without them? >>> >>> There should not be much redundancy since BufferingTCPClientSocket can >>> still inherit from BufferingStreamSocket. >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/passthrough.h:27: : >>> base_socket_(impl.Pass()) {} >>> nit: (everywhere) New code should use std::move instead of Pass() >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/socket/tcp_socket.h (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:23: typedef >>> base::Callback<void(void)> PauseCompletionCallback; >>> nit: please use a type alias instead of typedef now >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:33: class SocketPauseBuffer { >>> This class should be in its own header and cc file >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:35: typedef base::Callback< >>> nit: type alias >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:89: void >>> InsureBufferCanHold(int num_bytes); >>> nit: s/Insure/Ensure/ >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:103: // BufferingStreamSocket >>> is a client of downstream_socket_. >>> downstream_socket_ doesn't exist >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:105: // >>> scoped_ptr<net::StreamSocket> downstream_socket_; >>> Remove this? >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:144: class >>> BufferingStreamSocket : public Passthrough<net::StreamSocket> { >>> This class should have its own header and cc >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:174: class >>> BufferingTCPClientSocket : public Passthrough<net::TCPClientSocket> { >>> This class should have its own header and cc >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tcp_socket.h:314: void ApiPauseComplete(); >>> nit: How about NotifyPauseComplete? >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/socket/tls_socket.cc (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tls_socket.cc:53: // Wrap the StreamSocket >>> in a BufferingTCPSocket, to support SetPaused(). >>> nit: s/BufferingTCPSocket/BufferingStreamSocket/ >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/socket/tls_socket.cc:54: >>> scoped_ptr<BufferingStreamSocket> buffer_sock( >>> nit: buffer_socket? No real value in abbreviating here >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:5: #include >>> <hash_set> >>> Thanks for adding this :) But it should be >>> "base/containers/hash_tables.h" (and down below with chromium headers) >>> >>> >>> https://codereview.chromium.org/494573002/diff/60001/extensions/browser/api/s... >>> extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:198: >>> client_sock->Pause(); >>> How about just making Pause take a callback? Or if that's too annoying >>> (i.e. too many existing callsites to update), how about introducing a >>> PauseAsync call which takes a callback? >>> >>> https://codereview.chromium.org/494573002/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
