Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: Fix erroneous debug message that h2 connection closed #5118

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Mar 18, 2020

Prior to this change in libcurl debug builds http2 stream closure was
erroneously referred to as connection closure.

Before:

  • nread <= 0, server closed connection, bailing

After:

  • nread == 0, stream closed, bailing

Closes #xxxx


curld -v https://httpbin.org/get https://httpbin.org/get

* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
...
* nread <= 0, server closed connection, bailing
* STATE: PERFORM => DONE handle 0x4acbc8; line 2195 (connection #0)
* multi_done
* Connection #0 to host httpbin.org left intact
* Expire cleared (transfer 0x4acbc8)
* STATE: INIT => CONNECT handle 0x4acbc8; line 1619 (connection #-5000)
* Found bundle for host httpbin.org: 0x477b70 [can multiplex]
* Re-using existing connection! (#0) with host httpbin.org
...

Curl_read calls the underlying read (http2_recv) which returns 0 when the stream is closed:

curl/lib/transfer.c

Lines 590 to 629 in b81e0b0

if(bytestoread) {
/* receive data from the network! */
result = Curl_read(conn, conn->sockfd, k->buf, bytestoread, &nread);
/* read would've blocked */
if(CURLE_AGAIN == result)
break; /* get out of loop */
if(result>0)
return result;
}
else {
/* read nothing but since we wanted nothing we consider this an OK
situation to proceed from */
DEBUGF(infof(data, "readwrite_data: we're done!\n"));
nread = 0;
}
if(!k->bytecount) {
Curl_pgrsTime(data, TIMER_STARTTRANSFER);
if(k->exp100 > EXP100_SEND_DATA)
/* set time stamp to compare with when waiting for the 100 */
k->start100 = Curl_now();
}
*didwhat |= KEEP_RECV;
/* indicates data of zero size, i.e. empty file */
is_empty_data = ((nread == 0) && (k->bodywrites == 0)) ? TRUE : FALSE;
/* NUL terminate, allowing string ops to be used */
if(0 < nread || is_empty_data) {
k->buf[nread] = 0;
}
else {
/* if we receive 0 or less here, the server closed the connection
and we bail out from this! */
DEBUGF(infof(data, "nread <= 0, server closed connection, bailing\n"));
k->keepon &= ~KEEP_RECV;
break;
}

curl/lib/http2.c

Lines 1730 to 1733 in b81e0b0

/* If this stream is closed, return 0 to signal the http routine to close
the connection */
if(stream->closed)
return 0;

I don't understand that comment. When the stream is closed that does not mean the connection should be closed.

Prior to this change in libcurl debug builds http2 stream closure was
erroneously referred to as connection closure.

Before:
* nread <= 0, server closed connection, bailing

After:
* nread == 0, stream closed, bailing

Closes #xxxx
@jay jay added the HTTP/2 label Mar 18, 2020
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've seen it fly by many times and been a bit annoyed so I'm glad you stepped up and fixed it!

@jay jay closed this in 347a374 Mar 18, 2020
@jay jay deleted the fix_h2_stream_closed_dbg_msg branch March 18, 2020 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants