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

connection shutdown, basics and first use #13904

Closed
wants to merge 3 commits into from

Conversation

icing
Copy link
Contributor

@icing icing commented Jun 7, 2024

This adds connection shutdown infrastructure and first use for FTP. FTP data connections, when not encountering an error, are now shut down in a blocking way with a 2sec timeout.

  • add cfilter Curl_cft_shutdown callback
  • keep a shutdown start timestamp and timeout at connectdata
  • provide shutdown timeout default and member in
    data->set.shutdowntimeout.
  • provide methods for starting, interrogating and clearing shutdown timers
  • provide Curl_conn_shutdown_blocking() to shutdown the
    sockindex filter chain in a blocking way. Use that in FTP.
  • add Curl_conn_cf_poll() to wait for socket events during
    shutdown of a connection filter chain.
    This gets the monitoring sockets and events via the filters
    "adjust_pollset()" methods. This gives correct behaviour when
    shutting down a TLS connection through a HTTP/2 proxy.
  • Implement shutdown for all socket filters
    • for HTTP/2 and h2 proxying to send GOAWAY
    • for TLS backends to the best of their capabilities
    • for tcp socket filter to make a final, nonblocking receive to avoid unwanted RST states
    • add shutdown forwarding to happy eyeballers and https connect ballers when applicable.

Test coverage of the FTP blocking shutdown of its data connection is covered in test_30_5 and test_31_05 ftp upload tests.

Update: removed known bugs 7.5 and 7.11 fixed by this PR. Verified by local upload tests with vsftpd and confirmation on #13556.

This adds connection shutdown infrastructure and first use
for FTP. FTP data connections, when not encountering an error,
are now shut down in a blocking way with a 2sec timeout.

    - add cfilter `Curl_cft_shutdown` callback
    - keep a shutdown start timestamp and timeout at connectdata
    - provide shutdown timeout default and member in
      `data->set.shutdowntimeout`.
    - provide methods for starting, interrogating and clearing
      shutdown timers
    - provide `Curl_conn_shutdown_blocking()` to shutdown the
      `sockindex` filter chain in a blocking way. Use that in FTP.
    - add `Curl_conn_cf_poll()` to wait for socket events during
      shutdown of a connection filter chain.
      This gets the monitoring sockets and events via the filters
      "adjust_pollset()" methods. This gives correct behaviour when
      shutting down a TLS connection through a HTTP/2 proxy.
    - Implement shutdown for all socket filters
      - for HTTP/2 and h2 proxying to send GOAWAY
      - for TLS backends to the best of their capabilities
      - for tcp socket filter to make a final, nonblocking
        receive to avoid unwanted RST states
    - add shutdown forwarding to happy eyeballers and
      https connect ballers when applicable.
lib/cf-socket.c Outdated Show resolved Hide resolved
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.

It looks good. Just some questions!

}

timediff_t Curl_shutdown_timeleft(struct connectdata *conn, int sockindex,
struct curltime *nowp)
Copy link
Member

Choose a reason for hiding this comment

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

This function is used in two places, both of them set nowp to NULL. Do you really need this argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I follow the spirit of Curl_timeleft(). Shoudl we remove it there as well?

Copy link
Member

Choose a reason for hiding this comment

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

In Curl_timeleft's case there are several callers that use the argument so there it does save a few calls to Curl_now().

@@ -142,6 +142,43 @@ timediff_t Curl_timeleft(struct Curl_easy *data,
return (ctimeleft_ms < timeleft_ms)? ctimeleft_ms : timeleft_ms;
}

void Curl_shutdown_start(struct Curl_easy *data, int sockindex,
struct curltime *nowp)
Copy link
Member

Choose a reason for hiding this comment

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

This function is used in two places, both of them set nowp to NULL. Do you really need this argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

@@ -168,6 +177,61 @@ void Curl_conn_close(struct Curl_easy *data, int index)
}
}

CURLcode Curl_conn_shutdown_blocking(struct Curl_easy *data, int sockindex)
Copy link
Member

Choose a reason for hiding this comment

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

This risks blocking the multi interface for the entire timeout period (DEFAULT_SHUTDOWN_TIMEOUT_MS) when the FTPS data connection is closed. Do you plan to add handling of this in a non-blocking way in a follow-up change?

Copy link
Contributor Author

@icing icing Jun 9, 2024

Choose a reason for hiding this comment

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

To have a non-blocking shutdown of the data connection would be nice, indeed. That would require some more radical change in the FTP code, I guess.

For now, the blocking way at least assures that the FTP data connection works. We have multiple reports that at least uploads fail regularly if we do not do this, as FTP servers have become more sensitive to a clean TLS shutdown.

The change in this PR is a bugfix at the expense of parallel performance.

Note that the same concerns apply to the connect phase of FTP data connection where we do a blocking connect.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Let's merge this as it fixes issues, and maybe work on improving it further later on.

@bagder bagder closed this in c31041b Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants