conn vs data cleanup #3442

bagder · 2019-01-05T23:22:50Z

Easy handles and connections

History

A common pattern in the libcurl code is having connections (struct
connectdata) point to its corresponding transfer (easy handle) using the
conn->data struct member.

We therefore pass around the 'conn' pointer only in a lot of places as we can
defer the transfer from it (using conn->data).

This made perfect sense back in the days when each transfer had a single
connection for the duration of a transfer and became less good when we added
Pipelining and downright ugly when we added support for multiplexed
connections.

Now, several simultaneous transfers can use the same connection so conn->data
has to be updated very frequently to make it accurate for every possible
situation. This is error-prone and is repeteadly a reason for issues and bugs.

Two transfers using two connections

    /------------\          /--------------\
    | Transfer 1 |   ===>   | Connection 1 |
    | 'data'     |          \--------------/
    \------------/

    /------------\
    | Transfer 2 |          /--------------\
    | 'data'     |   ===>   | Connection 2 |
    \------------/          \--------------/

Two transfers using one connection

    /------------\          /--------------\
    | Transfer 1 |   ===>   | Connection 1 |
    | 'data'     |     -->  \--------------/
    \------------/    /
                     /
    /------------\  /
    | Transfer 2 | /
    | 'data'     |
    \------------/

A better way

A transfer however only uses a single connection for a transfer (or none at
all) at any given time. data->conn gets assigned when a connection is setup
and it gets cleared when the connection is again closed or returned to the
connection pool. Several transfers can point to the same connection while they
multiplex over the same one.

Going forward

Use of 'conn->data' should be removed over time in preference to 'data->conn'.
Function prototypes should rather work with struct Curl_easy *data as the
identifier poiting out the specific transfer and 'data->conn' to get the
connection (which then is NULL if there is no associated connection for this
transfer), rather than the old style that passes in struct connectdata *conn
to most functions.

Curl_attach_connnection() - this is the new function that gets called when
a connection is "attached" to a transfer. When the two gets their
association. The association is then kept until detached at a later point.
Curl_detach_connnection() - this is the new function that gets called to
separate the tranfer from its associated connection. From this point on,
the transfer has no longer any associated connection.

Gradual, not single-shot

I previously made an attempt in making this change (basically removing
conn->data) in a single shot, a single pull-request, but even before it was
completed the changeset reached over 3000 modified lines. Quite simply too
large to be reviewable and landable in one atomic change.

That experience made me draw the conclusion that we need to make this change
gradually and over time.

Over time, data->conn should be used and conn->data use should be reduced.

jay · 2019-01-07T08:02:22Z

I think it will probably be a bit rocky regardless. As it is it looks fine to me however CI re Curl_connect create_conn does not always allocate for conn.

bagder · 2019-01-07T08:11:11Z

I think it will probably be a bit rocky regardless.

Probably, yes. But I still think doing it gradually instead of a huge single merge will make a softer landing.

As it is it looks fine to me however CI re Curl_connect create_conn does not always allocate for conn.

Clearly some more fixes are needed to make the builds green...

We use "conn" everywhere to be a pointer to the connection. Introduces two functions that "attaches" and "detaches" the connection to and from the transfer. Going forward, we should favour using "data->conn" (since a transfer always only has a single connection or none at all) to "conn->data" (since a connection can have none, one or many transfers associated with it and updating conn->data to be correct is error prone and a frequent reason for internal issues). Closes #3442

We use "conn" everywhere to be a pointer to the connection. Introduces two functions that "attaches" and "detaches" the connection to and from the transfer. Going forward, we should favour using "data->conn" (since a transfer always only has a single connection or none at all) to "conn->data" (since a connection can have none, one or many transfers associated with it and updating conn->data to be correct is error prone and a frequent reason for internal issues). Side-effect: this also removes some old pipelining logic that isn't used and is destined for removal in April 2019 according to DEPRECATE.md. Closes #3442

bagder · 2019-01-10T09:08:02Z

Fixed the merge conflicts #3448 caused.

- Split off connection shutdown procedure from Curl_disconnect into new function conn_shutdown. - Change the shutdown procedure to close the sockets before disassociating the transfer. Prior to this change the sockets were closed after disassociating the transfer so SOCKETFUNCTION wasn't called since they were closed. That likely came about from recent work started in Jan 2019 (curl#3442) to separate transfers from connections. Bug: https://curl.haxx.se/mail/lib-2019-02/0101.html Reported-by: Pavel Löbl Closes curl#3597 Closes #xxxx

- Split off connection shutdown procedure from Curl_disconnect into new function conn_shutdown. - Change the shutdown procedure to close the sockets before disassociating the transfer. Prior to this change the sockets were closed after disassociating the transfer so SOCKETFUNCTION wasn't called since the transfer was already disassociated. That likely came about from recent work started in Jan 2019 (#3442) to separate transfers from connections. Bug: https://curl.haxx.se/mail/lib-2019-02/0101.html Reported-by: Pavel Löbl Closes #3597 Closes #3598

bagder force-pushed the bagder/conn-data-cleanup branch from b74e1f0 to 5c022f8 Compare January 5, 2019 23:26

bagder force-pushed the bagder/conn-data-cleanup branch 2 times, most recently from a60166a to 4dcc400 Compare January 9, 2019 14:16

bagder force-pushed the bagder/conn-data-cleanup branch from 4dcc400 to 3abec02 Compare January 10, 2019 09:04

bagder force-pushed the bagder/conn-data-cleanup branch from 3abec02 to 152a81e Compare January 10, 2019 09:07

bagder closed this in ba24323 Jan 11, 2019

bagder deleted the bagder/conn-data-cleanup branch January 13, 2019 22:11

jay mentioned this pull request Feb 21, 2019

url: change conn shutdown order to unlink data as last step #3598

Closed

lock bot locked as resolved and limited conversation to collaborators Apr 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conn vs data cleanup #3442

conn vs data cleanup #3442

bagder commented Jan 5, 2019

jay commented Jan 7, 2019

bagder commented Jan 7, 2019

bagder commented Jan 10, 2019

conn vs data cleanup #3442

conn vs data cleanup #3442

Conversation

bagder commented Jan 5, 2019

Easy handles and connections

History

Two transfers using two connections

Two transfers using one connection

A better way

Going forward

Gradual, not single-shot

jay commented Jan 7, 2019

bagder commented Jan 7, 2019

bagder commented Jan 10, 2019