Detect ping/pong flow-control abilities properly #9642

stefwalter · 2018-07-11T16:52:23Z

There is a rare situation, where cockpit-ws is of an older version
than a cockpit-bridge/cockpit.js pair (always shipped in same package).

In this situation, if flow control is used for a data heavy sideband
channel, there is the likelyhood that the channel will slow and
hang due to flow control. This is because cockpit-ws is not new
enough to know to respond to "ping" messages with "pong" messages.

To combat this, we expect to first see a "ping" message in a channel
before throttling senders for that channel. To facilitate this we
send a "ping" message as one of the first control messages in a
channel.

We still follow the general mechanism of sending a "ping" message
every N sequence bytes ... it's just now that we also do that when
N == 0.

This is a very small code change, with a very big change to the tests.
In order to facilitate backporting these patches I have stayed away
from refactoring the tests.

stefwalter · 2018-07-11T16:57:18Z

src/common/cockpitchannel.c

-    {
-      self->priv->out_sequence = 0;
-      self->priv->out_window = CHANNEL_FLOW_WINDOW;
-    }


Whoops, this change is unrelated.

allisonkarlitskaya · 2018-07-12T15:12:24Z

I don't understand what's going on here. Shouldn't we rather send a ping at N==0 and expect to see the first pong in response, before applying backpressure? Or do we expect both sides to now start out by sending 'ping' to each other? That would certainly be a bigger change in behaviour, at least for one-directional flows.

stefwalter · 2018-07-12T15:53:42Z

I don't understand what's going on here. Shouldn't we rather send a ping at N==0 and expect to see the first pong in response, before applying backpressure?

We apply back-pressure when the window is full.

However we turn that off completely, if we never see a channel "ping" from the other side.

Or do we expect both sides to now start out by sending 'ping' to each other?

Yes, both sides do ping each other at sequence % N == 0 as before.

allisonkarlitskaya · 2018-07-12T16:09:47Z

And there's no such thing as a single-directional channel?

stefwalter · 2018-07-12T16:18:39Z

And there's no such thing as a single-directional channel?

Not according to the protocol, no. Obviously many types channels send data in only or mostly in one direction, but the protocol does not declare or restrict this.

stefwalter · 2018-07-12T17:13:12Z

I just imagine a situation where someone is using ping for diagnostic purposes (ie: not flow control) and can therefore bring down an entire channel by accident.

The only requirement that flow control places on "ping" is that they're replied to with "pong" and the same contents. I can think of no scenario where a channel would go down by accident, if pings are used for diagnostic purposes.

stefwalter · 2018-07-12T17:13:45Z

In addition, nothing previously sent "ping" as a "channel" control message.

But I'm happy to adapt my pull request. Do you have specific feedback on what I would change it to?

allisonkarlitskaya · 2018-07-12T17:31:16Z

I imagine (admittedly) a pretty contrived situation where someone opens a JavaScript console or something and sends ping to debug a channel, which causes the other end to assume that this channel is now flow-control-capable on the other end.

Feel free to ignore this feedback, or we can chat about it tomorrow.

allisonkarlitskaya · 2018-07-13T13:59:52Z

summary of IRC: It's better to do this with a new field sent as part of the open command which indicates that the opposite side will respond to pings with pongs (ie: supports flow control). That should also make the patch much smaller because it will reduce the thrashing on the tests.

martinpitt · 2018-07-14T21:19:19Z

Might this be related to the test hang on rhel-7-5, see issue #9662? This sounds rather plausible, given that rhel-7-5 uses an old cockpit-bridge and -ws.

stefwalter · 2018-07-16T10:05:37Z

Did fix up based on discussion with @allisonkarlitskaya

Carrying a "flow-control" option in perpetuity just for this purpose is not a beautiful solution. But it's acceptable. In addition we can change the default for this later.

But why do I see "Rebase failed" everywhere here?

allisonkarlitskaya

This looks really quite a lot nicer. Thanks!

allisonkarlitskaya · 2018-07-16T10:33:30Z

src/common/cockpitchannel.c

+  /*
+   * The default here, can change from FALSE to TRUE over time once we assume that all
+   * cockpit-ws participants have been upgraded sufficiently. The default when we're
+   * on the channel creation side is to handle flow control.


Probably once we make this decision, we can just revert this patch, in fact. I can't imagine we'll ever have someone sending "{"flow-control": false}".

allisonkarlitskaya · 2018-07-16T10:35:39Z

src/ws/cockpitchannelresponse.c

@@ -395,6 +395,8 @@ cockpit_channel_response_prepare (CockpitChannel *channel)
  const gchar *payload;
  JsonObject *open;

+  COCKPIT_CHANNEL_CLASS (cockpit_channel_response_parent_class)->prepare (channel);


Maybe add the previously-missing chain-up as a separate commit?

In the CockpitChannelResponse implementation we need to call the base class prepare virtual function from the overridden one.

There is a rare situation, where cockpit-ws is of an older version than a cockpit-bridge/cockpit.js pair (always shipped in same package). In this situation, if flow control is used for a data heavy sideband channel, there is the likelyhood that the channel will slow and hang due to flow control. This is because cockpit-ws is not new enough to know to respond to "ping" messages with "pong" messages. To combat this, we expect to first see a "ping" message in a channel before throttling senders for that channel. To facilitate this we send a "ping" message as one of the first control messages in a channel. We still follow the general mechanism of sending a "ping" message every N sequence bytes ... it's just now that we also do that when N == 0. This is a very small code change, with a very big change to the tests. In order to facilitate backporting these patches I have stayed away from refactoring the tests.

martinpitt · 2018-07-16T19:22:09Z

src/base1/cockpit.js

@@ -841,6 +841,7 @@ function Channel(options) {
        else
            delete command.binary;

+        command["flow-control"] = true;


Just to understand -- the binary cockpit-bridge and thus the protocol default to "off" -- but the JavaScript API defaults to on, so that as soon as one uses a newer cockpit-system they get flow control enabled for everything. This would still work with new cockpit-system and old cockpit-bridge as unknown options get ignored. Did I understand that correctly?

Yes, that's correct.

martinpitt

@allisonkarlitskaya is out for a few days, and you addressed her feedback already. Looks good to me, thanks! I adjust the commit message on merging.

martinpitt requested a review from allisonkarlitskaya July 11, 2018 16:54

stefwalter commented Jul 11, 2018

View reviewed changes

stefwalter force-pushed the ping-feature-detection branch from 5953dc7 to 0efd56f Compare July 11, 2018 18:15

This was referenced Jul 14, 2018

Image refresh for rhel-7-5 #9655

Merged

testFlowControl hangs indefinitely on rhel-7-5 #9662

Closed

stefwalter force-pushed the ping-feature-detection branch from ad09bfd to b24fe19 Compare July 16, 2018 09:58

allisonkarlitskaya reviewed Jul 16, 2018

View reviewed changes

stefwalter added 3 commits July 16, 2018 13:42

ws: Fix missing chain up of prepare vfunc

306c94b

In the CockpitChannelResponse implementation we need to call the base class prepare virtual function from the overridden one.

FIXUP Add "flow-control" option

7c42414

stefwalter force-pushed the ping-feature-detection branch from b24fe19 to 7c42414 Compare July 16, 2018 11:47

martinpitt reviewed Jul 16, 2018

View reviewed changes

martinpitt requested a review from allisonkarlitskaya July 17, 2018 08:36

martinpitt approved these changes Jul 18, 2018

View reviewed changes

martinpitt closed this in 43a39d8 Jul 18, 2018

larskarlitski mentioned this pull request Jan 17, 2019

Failing to serve large files #7162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect ping/pong flow-control abilities properly #9642

Detect ping/pong flow-control abilities properly #9642

stefwalter commented Jul 11, 2018

stefwalter Jul 11, 2018

allisonkarlitskaya commented Jul 12, 2018

stefwalter commented Jul 12, 2018

allisonkarlitskaya commented Jul 12, 2018

stefwalter commented Jul 12, 2018

stefwalter commented Jul 12, 2018

stefwalter commented Jul 12, 2018 •

edited

allisonkarlitskaya commented Jul 12, 2018

allisonkarlitskaya commented Jul 13, 2018

martinpitt commented Jul 14, 2018

stefwalter commented Jul 16, 2018

allisonkarlitskaya left a comment

allisonkarlitskaya Jul 16, 2018

allisonkarlitskaya Jul 16, 2018

stefwalter Jul 16, 2018

martinpitt Jul 16, 2018

stefwalter Jul 17, 2018

martinpitt left a comment

Detect ping/pong flow-control abilities properly #9642

Detect ping/pong flow-control abilities properly #9642

Conversation

stefwalter commented Jul 11, 2018

stefwalter Jul 11, 2018

Choose a reason for hiding this comment

allisonkarlitskaya commented Jul 12, 2018

stefwalter commented Jul 12, 2018

allisonkarlitskaya commented Jul 12, 2018

stefwalter commented Jul 12, 2018

stefwalter commented Jul 12, 2018

stefwalter commented Jul 12, 2018 • edited

allisonkarlitskaya commented Jul 12, 2018

allisonkarlitskaya commented Jul 13, 2018

martinpitt commented Jul 14, 2018

stefwalter commented Jul 16, 2018

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

allisonkarlitskaya Jul 16, 2018

Choose a reason for hiding this comment

allisonkarlitskaya Jul 16, 2018

Choose a reason for hiding this comment

stefwalter Jul 16, 2018

Choose a reason for hiding this comment

martinpitt Jul 16, 2018

Choose a reason for hiding this comment

stefwalter Jul 17, 2018

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

stefwalter commented Jul 12, 2018 •

edited