Bootstrap websocket connection with http2 CONNECT #1625
Conversation
There was a problem hiding this comment.
The only confusing part is freeing request. Maybe just a good comment is required. Possible memory leaks do require a lot of attention.
Also please consider this patch for coding style cleanup style.diff.txt
|
In response to #1625 (review) I have applied suggested style amendments. |
There was a problem hiding this comment.
Basically there are the same questions: it seems with this patch set we free messages in smaller number of cases (under more narrow conditions), but do not introduce new places to free the messages - this looks like a memory leak.
I'd suggest to make an instrumentation patch with a counter for currently allocated/freed messages and verify websockets and say regular loading of our website through HTTP/2.
Also could you please elaborate on the problem of HTTP/2 client sending data right after Upgrade and immediately closing the stream - where the problem appears? Ideally with the sequence of called functions.
| * cases controlled by server connection side (after adding to | ||
| * @fwd_queue): successful response sending, eviction etc. | ||
| */ | ||
| if (!test_bit(TFW_HTTP_B_FULLY_PARSED, hmreq->flags) && !hmreq->resp) | ||
| tfw_http_conn_msg_free(hmreq); |
There was a problem hiding this comment.
Does !hmreq->resp implies websockets? If so, then we should make an assertion that this is a websocket request, otherwise this isn't clear. If not, then where the request with empty resp is freed now for regular HTTP/2 messages?
There was a problem hiding this comment.
To my understanding for regular requests request cannot have linked response and at the same time be not fully parsed. So that change do not narrow set of freed reqeusts here.
In our settings i suppose that only h2 websockets will have linked response here. I'll add assert.
| @@ -936,7 +950,10 @@ tfw_h2_resp_fwd(TfwHttpResp *resp) | |||
|
|
|||
| tfw_hpack_enc_release(&ctx->hpack, resp->flags); | |||
|
|
|||
| tfw_http_resp_pair_free(req); | |||
| if (tfw_http_resp_is_websocket_upgrade(resp)) | |||
| tfw_http_conn_msg_free(req->pair); | |||
There was a problem hiding this comment.
The question is still the same: if we don't free resp for webosockets here, then we do we free it?
|
In response to #1625 (comment) My idea is that in connection drop requests that linked to stream will be freed. |
ttaym
force-pushed
the
am-755-websocket-bootstrap-http2
branch
from
b7f4bf5
to
f9e7527
Compare
|
Decided not to go with Webosckets over HTTP/2 #755 (comment) |
Contributes to #755
This PR adds CONNECT method and
:protocolheader field for HTTP/2 parsing.CONNECT HTTP/2 messages with
:protocolequals towebsockettransformed to HTTP/1.1 messages for backend with method GET, headersConnection: upgradeandUpgrade: websocket.Responses from backend with headers
Connection: upgrade,Upgrade: websocketand status 101 transformed to:status200 HTTP/2 responses to client.When sending 200-success to CONNECT request with websocket
:protocolwe need not to send END_STREAM frame flag. So to gracefully shutting down stream we need to detectClosewebsocket control frame or rely on closing tcp connection from backend or closing stream from client.Note that
Sec-WebSocket-Acceptstripped from HTTP/1.1 responses andSec-WebSocket-Keyadded to HTTP/1.1 request to backend.Signed-off-by: Aleksey Mikhaylov aym@tempesta-tech.com