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

Ftp self connect mode #1704

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

FanDjango
Copy link
Collaborator

@FanDjango FanDjango commented Jan 8, 2025

Referring to #683 (long read but important)

#683 reported that accessing certain properties (specifically Capabilities and Hash Algorithms) when the (async)client is not connected would cause a cross-Async->Sync call to (sync)Connect(). That analysis was correct (at that time).

This was a result of extensive code changes and additions in the async client.

The (rather quick) fix was to remove any call to connect() in the property getters.

Later on in the history of development, reconnects due to stale data and just generally allowing API calls without a preceeding connect led to a certain confusion about which APIs are needing a connect in the first place (or not).

#1698 Reported the errors that can occur when accessing properties without a prior connect.

Addressing these was a bit involved.

This PR brings back the premature removal of connect() calls in certain property getters and introduces a config setting to steer the availability of "automatic" (re-)connects. It overcomes the danger of a cross-Async->Sync call to (sync)Connect(), which was the original problem identified by #683, not by a quick fix of just removing the connect calls but rather coding them in such a way that correct functionality is guaranteed.

The new config setting:

		/// <summary>
		/// Configure the behaviour of the Self Connect feature for the control connection
		/// Always (default): If the control connection is needed to process an API process (either for entering commands to the server
		/// or to retrieve information about the server (FEAT capabilities, HASH algorithms, Current Working directory) it will be (re-)
		/// connected if not available.
		/// OnConnectionLost: As with "Always", BUT only if there had been a connection in the first place before it got lost.
		/// Never: Connections will not be made unless you explicitly call Connect(...) yourself. If a connection is needed and not
		/// established, your API processes will fail instead of attempting to (re-)connect.
		/// </summary>
		public FtpSelfConnectMode SelfConnectMode { get; set; } = FtpSelfConnectMode.Always;

The new config enum for client.Config.SelfConnectMode is the following and it keeps things ASIS as the default is "Always":

	public enum FtpSelfConnectMode {

		/// <summary>
		/// Always (default): If the control connection is needed to process an API process (either for entering commands to the server
		/// or to retrieve information about the server (FEAT capabilities, HASH algorithms, Current Working directory) it will be (re-)
		/// connected if not available.
		/// </summary>
		Always,

		/// <summary>
		/// OnConnectionLost: As with "Always", BUT only if there had been a connection in the first place before it got lost.
		/// </summary>
		OnConnectionLost,

		/// <summary>
		/// Never: Connections will not be made unless you explicitly call Connect(...) yourself. If a connection is needed and not
		/// established, your API processes will fail instead of attempting to (re-)connect.
		/// </summary>
		Never,
	}

As a second stage in the future, it will be necessary to migrate the two property getters, Capabilites and Hash Algorithms to be
methods instead of properties. Such a migration is also pending (as a further example) for the IsConnected property of the FTPSocketStream.

Such getters are not really supposed to invoke complex operations, although there is no formal rule against this. In all three cases (FtpSocketStream.IsConnected, client.Capabilites, client.HashAlgorithms) converting these to methods is quite involved).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant