Skip to content

H2 to origin for 10-Dev branch#8447

Closed
shinrich wants to merge 4 commits intoapache:10-Devfrom
shinrich:server-h2-origin-10-Dev
Closed

H2 to origin for 10-Dev branch#8447
shinrich wants to merge 4 commits intoapache:10-Devfrom
shinrich:server-h2-origin-10-Dev

Conversation

@shinrich
Copy link
Member

A continuation of PR #7622 but against the 10-Dev branch instead of master. Includes fixes from several months of production testing.

@shinrich shinrich added this to the 10.0.0 milestone Oct 25, 2021
@shinrich shinrich requested review from bneradt and maskit October 25, 2021 11:09
@shinrich shinrich self-assigned this Oct 25, 2021
@shinrich shinrich mentioned this pull request Oct 25, 2021
@bneradt
Copy link
Contributor

bneradt commented Oct 25, 2021

Thank you @shinrich for creating this.

This patch will fix the compiler error raised by the AuTest and Ubuntu test runs:

$ git show 90633ed935291da0e0a74e2720e0aaddf5c8dd53
commit 90633ed935291da0e0a74e2720e0aaddf5c8dd53
Author: Brian Neradt <brian.neradt@verizonmedia.com>
Date:   Thu Sep 30 19:39:04 2021 +0000

    Fix a compiler error and unused parameter warning.

diff --git a/proxy/http2/Http2CommonSession.h b/proxy/http2/Http2CommonSession.h
index 075d7c2..e81dbda 100644
--- a/proxy/http2/Http2CommonSession.h
+++ b/proxy/http2/Http2CommonSession.h
@@ -184,7 +184,7 @@ Http2CommonSession::is_url_pushed(const char *url, int url_len)
     return false;
   }
 
-  return _h2_pushed_urls->find(url) != _h2_pushed_urls->end();
+  return _h2_pushed_urls->find(std::string{url, static_cast<size_t>(url_len)}) != _h2_pushed_urls->end();
 }
 
 inline int64_t
diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc
index 859b0b9..1b17803 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -1415,7 +1415,7 @@ Http2ConnectionState::create_initiating_stream(bool client_streamid, Http2Error
                                               peer_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE), true, true);
 
   ink_assert(nullptr != new_stream);
-  !stream_list.in(new_stream);
+  ink_assert(!stream_list.in(new_stream));
 
   stream_list.enqueue(new_stream);
   if (client_streamid) {

This patch will address an assertion we were tripping (relatively rarely) in production after merging in these changes:

diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index bcffe07..1d60806 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -404,12 +404,15 @@ ConnectingEntry::state_http_server_open(int event, void *data)
   case VC_EVENT_ACTIVE_TIMEOUT:
   case VC_EVENT_ERROR:
   case NET_EVENT_OPEN_FAILED: {
-    ink_release_assert(_netvc != nullptr);
     Debug("http_connect", "Stop %" PRId64 " state machines waiting for failed origin", _connect_sms.size());
     this->remove_entry();
-    int vc_provided_cert = _netvc->provided_cert();
-    int lerrno           = _netvc->lerrno;
-    _netvc->do_io_close();
+    int vc_provided_cert = 0;
+    int lerrno           = EIO;
+    if (_netvc != nullptr) {
+      vc_provided_cert = _netvc->provided_cert();
+      lerrno           = _netvc->lerrno;
+      _netvc->do_io_close();
+    }
     while (!_connect_sms.empty()) {
       auto entry = _connect_sms.begin();
       SCOPED_MUTEX_LOCK(lock, (*entry)->mutex, this_ethread());

@keesspoelstra
Copy link
Contributor

It needs shinrich@344a1de as well

@bneradt
Copy link
Contributor

bneradt commented Oct 25, 2021

Thank you Susan for putting this up. I'll take this over and replace it with: #8448

@bneradt bneradt closed this Oct 25, 2021
@bneradt bneradt mentioned this pull request Jul 14, 2022
@zwoop zwoop removed this from the 10.0.0 milestone Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants