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

pipelining: check for doomed connections #1075

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@nopjmp

nopjmp commented Oct 14, 2016

Issue #627: Checks for doomed connections when selecting a connection for pipelining. Restores checks by Carlo Wood.

It appears that Rider-Linden hasn't had time to upstream this. We were bitten by this bug with our fork of their project and found this commit message as I can replicate the problem without issue on ethernet over powerline within my home. They have been using this fix / workaround for several months now.

So, I took the liberty of getting his fork rebased into a single commit and will fix any code quality changes that are recommended by the community.

Issue #627: Checks for doomed connections when selecting a connection…
… for pipelining. Restores checks by Carlo Wood.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 14, 2016

@nopjmp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @captain-caveman2k to be potential reviewers.

mention-bot commented Oct 14, 2016

@nopjmp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @captain-caveman2k to be potential reviewers.

@jay

style changes

Show outdated Hide outdated lib/url.c
@@ -2908,7 +2908,8 @@ static bool IsPipeliningPossible(const struct Curl_easy *handle,
const struct connectdata *conn)
{
/* If a HTTP protocol and pipelining is enabled */
if(conn->handler->protocol & PROTO_FAMILY_HTTP) {
if(!conn->bits.close &&
(conn->handler->protocol & PROTO_FAMILY_HTTP)) {

This comment has been minimized.

@jay

jay Oct 14, 2016

Member

please review code style column alignment, you need to back it up one see the other if statements in the function for example. also it could fit on one line, for example

  if((conn->handler->protocol & PROTO_FAMILY_HTTP) && !conn->bits.close) {
@jay

jay Oct 14, 2016

Member

please review code style column alignment, you need to back it up one see the other if statements in the function for example. also it could fit on one line, for example

  if((conn->handler->protocol & PROTO_FAMILY_HTTP) && !conn->bits.close) {
Show outdated Hide outdated lib/url.c
@@ -3283,6 +3284,12 @@ ConnectionExists(struct Curl_easy *data,
pipeLen = check->send_pipe->size + check->recv_pipe->size;
if(canPipeline) {
/*
* We can not use this connection if the connection is marked for

This comment has been minimized.

@jay

jay Oct 14, 2016

Member

comment is too many lines, though i know you were doing it like it is above. instead do it like

        /* If the connection is marked for closure it can't be used. */
        if(check->bits.close)
          continue;

although it seems superfluous to me, because the code implies that. you could probably eschew it

@jay

jay Oct 14, 2016

Member

comment is too many lines, though i know you were doing it like it is above. instead do it like

        /* If the connection is marked for closure it can't be used. */
        if(check->bits.close)
          continue;

although it seems superfluous to me, because the code implies that. you could probably eschew it

@nopjmp

This comment has been minimized.

Show comment
Hide comment
@nopjmp

nopjmp Oct 14, 2016

I agree. I was having a hard time justifying the large comment from the original patch, but I was leaving it in just in case. I also rearranged the checks like you did in the example as that made more sense to me regarding the comment before the check.

nopjmp commented Oct 14, 2016

I agree. I was having a hard time justifying the large comment from the original patch, but I was leaving it in just in case. I also rearranged the checks like you did in the example as that made more sense to me regarding the comment before the check.

@jay jay closed this in e5f0b1a Oct 14, 2016

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 14, 2016

Member

Thanks, just landed. I squashed it into the previous commit and gave you attribution in the commit message as 'Participation-by:'.

Member

jay commented Oct 14, 2016

Thanks, just landed. I squashed it into the previous commit and gave you attribution in the commit message as 'Participation-by:'.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 14, 2016

Member

breaks some pipelining tests, investigating

Member

jay commented Oct 14, 2016

breaks some pipelining tests, investigating

@jay jay reopened this Oct 14, 2016

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 16, 2016

Member

Background: All connections start off in the 'close' state and later their protocol can turn that off, for example HTTP will default to persistent connections when its protocol specific function is called.

However, before the protocol specific function is called create_conn calls ConnectionExists and IsPipeliningPossible and so now that we are checking for the close bit in those functions the connections are not always pipelined, causing some tests to fail (530 584 1900 1902 1903).

I'm not sure about the best way to solve this, I don't know if this should be reverted for the moment or there is some easy fix I'm missing. It seems to me that we would need a separate bit to mark a connection as "dead" "doomed" and then test that bit instead to make a proper fix.

Member

jay commented Oct 16, 2016

Background: All connections start off in the 'close' state and later their protocol can turn that off, for example HTTP will default to persistent connections when its protocol specific function is called.

However, before the protocol specific function is called create_conn calls ConnectionExists and IsPipeliningPossible and so now that we are checking for the close bit in those functions the connections are not always pipelined, causing some tests to fail (530 584 1900 1902 1903).

I'm not sure about the best way to solve this, I don't know if this should be reverted for the moment or there is some easy fix I'm missing. It seems to me that we would need a separate bit to mark a connection as "dead" "doomed" and then test that bit instead to make a proper fix.

@bagder bagder changed the title from Issue #627: Check for doomed connections to pipelining: check for doomed connections Oct 16, 2016

@bagder bagder added the HTTP label Oct 16, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 16, 2016

Member

These test failures are sort of proof why Pipelining is a pain to support in the first place (and I ultimately would like to remove that support in a future). The timing sensitivity is really hard to write reliable tests for. In this case, it really seems like the tests are to blame but I would feel very uncomfortable disabling these tests since Pipelining is already shaky as it is and reducing the number of pipelining tests further is like asking for more breakage to land.

This said, we can't have broken tests either so... I think we should revert the patch short-term and then see what we can do to fix the tests when trying to merge the fix back again.

Member

bagder commented Oct 16, 2016

These test failures are sort of proof why Pipelining is a pain to support in the first place (and I ultimately would like to remove that support in a future). The timing sensitivity is really hard to write reliable tests for. In this case, it really seems like the tests are to blame but I would feel very uncomfortable disabling these tests since Pipelining is already shaky as it is and reducing the number of pipelining tests further is like asking for more breakage to land.

This said, we can't have broken tests either so... I think we should revert the patch short-term and then see what we can do to fix the tests when trying to merge the fix back again.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 17, 2016

Member

@nopjmp I just made a follow up to this fix to relax it a little and allow connections that haven't yet reached the protocol level to be considered possible for pipelining. Since you are able to reproduce would you try aec0c99 (shouldn't crash) and also curl-7_50_3 (should) and confirm?

Member

jay commented Oct 17, 2016

@nopjmp I just made a follow up to this fix to relax it a little and allow connections that haven't yet reached the protocol level to be considered possible for pipelining. Since you are able to reproduce would you try aec0c99 (shouldn't crash) and also curl-7_50_3 (should) and confirm?

@nopjmp

This comment has been minimized.

Show comment
Hide comment
@nopjmp

nopjmp Oct 22, 2016

I did test this, but I forgot to respond. This fixes it better than the workaround Carlo Wood and others came up with at least on my horrible connection.

Thanks for getting the tests working again and finding a better condition to exit on.

nopjmp commented Oct 22, 2016

I did test this, but I forgot to respond. This fixes it better than the workaround Carlo Wood and others came up with at least on my horrible connection.

Thanks for getting the tests working again and finding a better condition to exit on.

@nopjmp nopjmp closed this Oct 22, 2016

@nopjmp nopjmp deleted the nopjmp:doomed-pipelining branch Oct 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment