Fix VST / HTTP2 idle connection keep alive behaviour #12549

graetzer · 2020-08-27T14:59:10Z

Scope & Purpose

Make VST and HTTP2 connection have the same keep-alive behaviour as a HTTP 1 connection. Namely only truely cancel an IDLE connection

💩 Bugfix

Backports:

Backports required for: 3.7

Should be more or less covered by existing tests
http://jenkins01.arangodb.biz:8080/job/arangodb-matrix-pr/11663/

…-173

graetzer · 2020-08-27T15:01:54Z

another one with go-tests enabled: http://jenkins01.arangodb.biz:8080/job/arangodb-matrix-pr/11664/

maierlars · 2020-08-28T07:53:50Z

3rdParty/fuerte/src/GeneralConnection.h

@@ -330,7 +330,7 @@ class GeneralConnection : public fuerte::Connection {
  Socket<ST> _proto;

  /// elements to send out
-  boost::lockfree::queue<RT*, boost::lockfree::capacity<64>> _queue;
+  boost::lockfree::queue<RT*, boost::lockfree::capacity<32>> _queue;


is there a reason why it was 64 and why it is now 32?

to match what happens on the server, the number is sort of arbitrary

maierlars · 2020-08-28T07:58:32Z

lib/Rest/CommonDefines.h

@@ -86,7 +86,7 @@ enum class EncodingType {
  UNSET
 };

-enum class AuthenticationMethod { BASIC, JWT, NONE };
+enum class AuthenticationMethod : uint8_t { BASIC = 1, JWT = 2, NONE = 0 };


Was this change necessary? Did I miss something?

maierlars · 2020-08-28T09:35:03Z

arangod/GeneralServer/HttpCommTask.cpp

+    }
+
+    auto& me = static_cast<HttpCommTask<T>&>(*s);
+    if ((wasReading && me._reading) ||


As I understand, this code will always call asyncReadNextRequest and thus _reading almost always true.

Now, previously I added the _requestCount which is now gone again. The reason was the following: imagine the a new request coming in and the timeout expires at the same time. Now both completion handlers are on the queue, first the read completion runs and decides to execute the request, and then restart reading. Now the timeout completion runs, sees that it is still reading and closes the request. Thus we have the same bug as before.

I think you are mixing these, this is the http 1.1 version there is no asyncReadNextRequest in this class

VST / HTTP2 now have their own setIOTimeout() implementation

_reading is not true unless we are onto the next request actually

Got it. I thouht we start reading the next request as soon as we called processRequest. However, that is indeed not the case here.

fceller · 2020-08-31T08:14:25Z

arangod/GeneralServer/H2CommTask.cpp

+  const bool wasWriting = this->_writing;
+  TRI_ASSERT(wasReading || wasWriting);
+  if (wasWriting) {
+    secs = std::max(300.0, secs);


Should be configurable?

jsteemann · 2020-08-31T09:43:58Z

arangod/GeneralServer/H2CommTask.cpp

+  const bool wasWriting = this->_writing;
+  TRI_ASSERT(wasReading || wasWriting);
+  if (wasWriting) {
+    secs = std::max(300.0, secs);


Why is the timeout at least 300 seconds here? Is there any specific reason for it?

because the idle timeout may be super low, so this is a 300 second write timeout

If it's a different timeout here (not an idle timeout, but a write timeout), shouldn't it be configurable?

jsteemann · 2020-08-31T09:48:55Z

arangod/GeneralServer/HttpCommTask.cpp

                         DTraceHttpCommTaskResponseWritten((size_t) self.get());

-                         auto* thisPtr = static_cast<HttpCommTask<T>*>(self.get());
+                         auto& me = static_cast<HttpCommTask<T>&>(*self);
+    me._writing = false;


indentation level looks strange here.

jsteemann · 2020-08-31T09:49:31Z

arangod/GeneralServer/HttpCommTask.cpp

-                         if (ec || !thisPtr->_shouldKeepAlive || err != HPE_PAUSED) {
-                           thisPtr->close(ec);
+                         llhttp_errno_t err = llhttp_get_errno(&me._parser);
+    if (ec || !me._shouldKeepAlive || err != HPE_PAUSED) {


indentation level looks strange here

jsteemann · 2020-08-31T09:50:46Z

arangod/GeneralServer/VstCommTask.cpp

+  const bool wasWriting = this->_writing;
+  TRI_ASSERT(wasReading || wasWriting);
+  if (wasWriting) {
+    secs = std::max(300.0, secs);


why is the timeout at least 300 seconds hee? the 300 seconds minimum seems to be in place for HTTP/2 and VST, but not for HTTP/1. I don't see a reason for this.

because i am not gonna change the http 1.1 behaviour

Ok, but then this still leaves us with protocol-specific behavior:

HTTP/1.1 doesn't have a write timeout

HTTP/2 and VST have a write timeout which is max(idle timeout, 300)

Ideally we wouldn't have differences between the protocols, but try to establish the same behavior for all protocols. If that requires a write-timeout to be configurable, we should add it as a new configuration option.

nah some people are sending GB image files as part of foxx apps and stuff like that (and we are not even using sendfile here). I'd rather keep it sane

maierlars · 2020-08-31T12:09:09Z

arangod/GeneralServer/HttpCommTask.cpp

+    }
+
+    auto& me = static_cast<HttpCommTask<T>&>(*s);
+    if ((wasReading && me._reading) ||


Got it. I thouht we start reading the next request as soon as we called processRequest. However, that is indeed not the case here.

…-173

graetzer · 2020-08-31T13:05:47Z

http://jenkins01.arangodb.biz:8080/job/arangodb-matrix-pr/11700/

jsteemann · 2020-08-31T17:35:34Z

LGTM except that the WriteTimeout is still hard-coded to at least 300 seconds.
I am fine with this being the default value, but I think we should make this value configurable via a startup option.

@fceller @graetzer : ideally we should also make it possible to enforce this write timeout for HTTP/1 connections too, because that would mean all protocols will have the same behavior.
As that would cause downwards-incompatibility for HTTP/1, this probably needs to be opt-in.
What about having the following options:

--http.request-timeout: config option to set this timeout
--http.request-timeout-behavior: values: "enforce", "compat", with "enforce" enforcing the timeout for all protocols, and "compat" enforcing it just for HTTP/2 and VST (downwards-compatible)

And in a future version, we can deprecate the --http.* options and rename them to something else, because they are used for VST as well. Probably something like --protocol.*?

graetzer · 2020-08-31T19:04:25Z

i don't know this is just a fix for buggy idle timeouts, i have too many diverging branches, I understand customers are waiting for this

jsteemann · 2020-08-31T22:31:23Z

Has this PR been obsoleted by #12579?
Because #12579 contains the changes made in this PR as well. 🤔

graetzer · 2020-08-31T22:40:37Z

idgaf if I have to wait for this PR to be merged I cannot work

jsteemann · 2020-08-31T23:03:51Z

@graetzer : I am confused now.
Is your preference to merge this PR here sooner than #12579, and merge devel into 12579 once we have merged this PR here?
Just not clear to me, so an explanation would help. Thanks!

(cherry picked from commit 8dfe68f)

* Fix VST / HTTP2 idle connection keep alive behaviour (#12549) (cherry picked from commit 8dfe68f) * remove wrong merge Co-authored-by: KVS85 <vadim@arangodb.com>

graetzer added 4 commits August 25, 2020 15:17

Only close true IDLE connections

00a825d

fix logid

b766acd

fix server timeouts

5d7c86c

Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/bts…

fe7a8a1

…-173

graetzer changed the title ~~Bug fix/bts 173~~ Fix VST / HTTP2 idle connection keep alive behaviour Aug 27, 2020

graetzer requested review from jsteemann and maierlars August 27, 2020 14:59

maierlars suggested changes Aug 28, 2020

View reviewed changes

fceller reviewed Aug 31, 2020

View reviewed changes

jsteemann reviewed Aug 31, 2020

View reviewed changes

maierlars approved these changes Aug 31, 2020

View reviewed changes

graetzer added 2 commits August 31, 2020 15:04

address review comments

74d4a86

Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/bts…

0c3d04a

…-173

graetzer requested a review from jsteemann August 31, 2020 13:53

graetzer mentioned this pull request Aug 31, 2020

Add fuerte testsuite #12579

Merged

8 tasks

jsteemann merged commit 8dfe68f into devel Sep 1, 2020

jsteemann deleted the bug-fix/bts-173 branch September 1, 2020 07:47

graetzer added a commit that referenced this pull request Sep 1, 2020

Fix VST / HTTP2 idle connection keep alive behaviour (#12549)

153f990

(cherry picked from commit 8dfe68f)

graetzer mentioned this pull request Sep 1, 2020

Fix VST / HTTP2 idle connection keep alive behaviour (#12549) #12584

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VST / HTTP2 idle connection keep alive behaviour #12549

Fix VST / HTTP2 idle connection keep alive behaviour #12549

graetzer commented Aug 27, 2020 •

edited

Loading

graetzer commented Aug 27, 2020

maierlars Aug 28, 2020

graetzer Aug 28, 2020

maierlars Aug 28, 2020

maierlars Aug 28, 2020

graetzer Aug 28, 2020

graetzer Aug 28, 2020

graetzer Aug 28, 2020

maierlars Aug 31, 2020

fceller Aug 31, 2020 •

edited

Loading

jsteemann Aug 31, 2020

graetzer Aug 31, 2020

jsteemann Aug 31, 2020

jsteemann Aug 31, 2020

jsteemann Aug 31, 2020

jsteemann Aug 31, 2020

graetzer Aug 31, 2020

jsteemann Aug 31, 2020

graetzer Aug 31, 2020

maierlars Aug 31, 2020

graetzer commented Aug 31, 2020

jsteemann commented Aug 31, 2020

graetzer commented Aug 31, 2020 •

edited

Loading

jsteemann commented Aug 31, 2020

graetzer commented Aug 31, 2020

jsteemann commented Aug 31, 2020

Fix VST / HTTP2 idle connection keep alive behaviour #12549

Fix VST / HTTP2 idle connection keep alive behaviour #12549

Conversation

graetzer commented Aug 27, 2020 • edited Loading

Scope & Purpose

Backports:

graetzer commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fceller Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graetzer commented Aug 31, 2020

jsteemann commented Aug 31, 2020

graetzer commented Aug 31, 2020 • edited Loading

jsteemann commented Aug 31, 2020

graetzer commented Aug 31, 2020

jsteemann commented Aug 31, 2020

graetzer commented Aug 27, 2020 •

edited

Loading

fceller Aug 31, 2020 •

edited

Loading

graetzer commented Aug 31, 2020 •

edited

Loading