It probably is, but I'd make sure m_stream.DisposeAsync / Stream.DisposeAsync is calling FtpSocketStream.Dispose.

from fluentftp.

Added some more small code changes to fix some glitches this caused in SYNC mode, of all places. Closing this now and thanks for the inpur

from fluentftp.

Thanks for reporting this. I will investigate.

from fluentftp.

Possible duplicate: #1460

from fluentftp.

I just tried this explicitly disposing an AsyncFtpClient in connected state (instead of calling Disconnect(..) ) and I get this:

Command:  PWD
Status:   Waiting for response to: PWD
Response: 257 "/" is current directory. [<1ms]
Press enter to continue

>         Dispose()
Status:   Disposing FtpClient object...
Status:   Closing/Disposing FtpSocketStream(control connection)
Finished

So I can't seem to reproduce it. What does your minimal code look like that produces your log?

from fluentftp.

Ooops. I see the exception, so disregard the previous post. Investigating some more...

from fluentftp.

As mentioned, it likely started with PR #1422 which involved interface work.

from fluentftp.

https://github.com/robinrodricks/FluentFTP/pull/1422/files#r1479021315

https://github.com/robinrodricks/FluentFTP/pull/1422/files#r1479030870

from fluentftp.

See my comment in https://github.com/robinrodricks/FluentFTP/pull/1422/files#r1479547199

Of course, the easy way out is to run all the work in Dispose() non-async, which would not be a problem if it were not for the Execute command hidden therein.

If you invoke the Dispose after you have disconnected, the dispose no longer invokes a disconnect, as it is unneccessary, so no problem.

But for an ASYNC client, I suppose one would expect to use
await client.Dispose(client, token);
instead of a simple dispose.

from fluentftp.

Sorry, I should have used different wording.

AsyncFtpClient doesn't have a dispose method, so it uses BaseFtpClient's Dispose, which casts to IInteralFtpClient (as pointed out in first comment), which then gets its DisconnectInternal called, which resolves to use BaseFtpClient's Disconnect (maybe because we are in the the Dispose of that class).

BaseFtpClient then casts to IFtpClient in all cases to call Disconnect (second PR comment), but we are an AsyncFtpClient, hence the InvalidCastException. A check of some sort needs to be made to see if a cast to IAsyncFtpClient or IFtpClient should be made for the Disconnect call.

If IAsyncFtpClient maybe just .GetAwaiter().GetResult() or Task.Run to not block since we don't care about the connection anymore anyways.

from fluentftp.

Sorry, I should have used different wording.

No, no, it's ok. I'm on the same track by now.

There are a few places where this check if (this is AsyncFtpClient) { .... would be needed, in fact.

But for the special case of Dispose - it wouldn't need this complication, as it should not make a hidden call to "Execute("QUIT)" (which is in Disconnect), if we are still connected.

For this case, I propose removing that. He who calls Dispose before Disconnect should not expect FluentFTP to issue a "QUIT" command prior to closing the connection, right?

Instead, I think that for client.Dispose() to make a hidden call to Execute("QUIT") in case IsConnected is true, is a nono.

from fluentftp.

If IAsyncFtpClient maybe just .GetAwaiter().GetResult() or Task.Run to not block since we don't care about the connection anymore anyways.

Nice idea, sort of unclean though.

from fluentftp.

Since we are slowly trying to get the kinks out of the ASYNC stuff, I wouldn't want "Dispose()" to block, firstly. And to potentially leave hanging tasks out there is sort of worrying, especially when one thinks of Dispose of "cleaning up nicely".

So I would rather remove the call to "DisconnectInternal" entirely, as a believe it is in the users responsibility to invoke Disconnect himself and not expect Dispose to do it for him. But then, others may be of a different opinion.

Otherwise I would really like a
await client.Dispose(token)
analogous to the SYNC way of doing things, since in this case Dispose might block.

from fluentftp.

The proper way to handle it would be to make AsyncFtpClient IAsyncDisposable & implement the DisposeAsync method:

https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync

I'm not calling Dispose manually, rather just exiting using blocks in async methods which the above would then handle.

A bit more work than just Task.Run 🚀 & 🤞 though.

from fluentftp.

Learning never ends. Thanks for this. This points me in the right direction.

from fluentftp.

I have merged a tenative "workaround" fix into the current master. It uses your suggestion of a ".GetAwaiter().GetResult()" and removes the exception.

I have made a note about "IAsyncDisposable".

It would open a can of worms:

from fluentftp.

Was worried about that but just covered my eyes and 🙏

What about in a new file AsyncClient/DisposeAsync.cs:

using // ...

#if NETSTANDARD2_1_OR_GREATER || NET5_0_OR_GREATER
public partial class AsyncFtpClient : IAsyncDisposable
{
   // ... DisposeAsync implementation  
}
#endif

Note: I have no idea if this is a viable solution as I don't have experience with how these preprocessor directives work with Nuget packaging.

Note 2: I also don't know if you can add on more interfaces in a partial class like this.

from fluentftp.

Although if the second note above doesn't work then we could just modify the original class definition:

#if ...
public partial class AsyncFtpClient : BaseFtpClient, IInternalFtpClient, IDisposable, IAsyncFtpClient, IAsyncDisposable
#else
public partial class AsyncFtpClient : BaseFtpClient, IInternalFtpClient, IDisposable, IAsyncFtpClient
#endif

from fluentftp.

Yeah, that is more or less what will be needed.

It's a pity as we were trying to get less and less of these, or at least restrict them to very low level near the system kind of stuff.

The entire attempt to extract common code from async and sync code is always endangered by the often found need to invoke a high level api function from down under and then needing to "de-segregate".

Re your latest post: You don't have time for a quick check? I'm inundated right now with other things.

from fluentftp.

Check regarding note 1 or 2?

As of yet I haven't even cloned the codebase, just used Resharper to decompiler the module while debugging the exception to step through the code and have been browsing on GitHub since.

from fluentftp.

Never mind for the moment. Let's sleep on it.

from fluentftp.

I've tested things. Two solutions look like they will work (they build without errors anyways). One requires preprocessor directives, the other requires adding references to 2 packages.

Note that the best solution in both cases also involves modifying FtpSocketStream to implement DisposeAsync, since it is a Stream that has async disposables.

Solution 1: Directives

AsyncClient/DisposeAsync.cs

using System;
using System.Threading.Tasks;

namespace FluentFTP;

#if NETSTANDARD2_1_OR_GREATER || NET5_0_OR_GREATER
public partial class AsyncFtpClient : IAsyncDisposable
{
	public async ValueTask DisposeAsync() {
		// Perform async cleanup.
		await DisposeAsyncCore();

		// Suppress finalization.
		GC.SuppressFinalize(this);
	}

	protected virtual async ValueTask DisposeAsyncCore() {
		if (IsDisposed) {
			return;
		}

		// Fix: Hard catch and suppress all exceptions during disposing as there are constant issues with this method
		try {
			LogFunction(nameof(DisposeAsync));
			LogWithPrefix(FtpTraceLevel.Verbose, "Disposing " + this.ClientType + " object...");
		}
		catch {
		}

		try {
			if (IsConnected) {
				await Disconnect();
			}
		}
		catch {
		}

		if (m_stream != null) {
			try {
				await m_stream.DisposeAsync();
			}
			catch {
			}

			m_stream = null;
		}

		try {
			m_credentials = null;
			m_textEncoding = null;
			m_host = null;
		}
		catch {
		}

		IsDisposed = true;
	}
}
#endif

Solution 2: PolySharp

PolySharp generates the missing language features for us, but it does also require a ref to the other package below to enable IAsyncDisposable use. Otherwise same code as above less the directives.

FluentFTP.csproj:

      <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="8.0.0" />
      <PackageReference Include="PolySharp" Version="1.14.1">
        <PrivateAssets>all</PrivateAssets>
        <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      </PackageReference>

from fluentftp.

also involves modifying FtpSocketStream to implement...

Absolutely. I just implemented the same sort of stuff (in the "easy" GetAwaiter flavor) for CloseDataStream. In that one, the async routine was completely missing as well.

PolySharp

I am pretty sure @robinrodricks would prefer to not use any (more) packages, so using the directives would be the way to go.

Thanks for the code. The normal sync dispose will also need a few lines to bring it up to par... as per the instruction from microsoft .-) and the "easy" code will need to be brought active when the AsyncDisposable is not viable.

I'll try to get on this soon.

Meanwhile there is a "primitive" fix merged into master to put out any fires.

from fluentftp.

Reverting to a lower version results in no such exception.

By the way, "Reverting to a lower version" by no means made the code execute asyncly - it probably reverted to sync further down the chain somewhere.

from fluentftp.

the "easy" code will need to be brought active when the AsyncDisposable is not viable.

The existing Dispose with synchronous code should get called automatically when the async version is not available, or if and when AsyncFtpClient is used outside outside of an async context. Implementing both IDisposable & IDisposableAsync is encouraged.

By the way, "Reverting to a lower version" by no means made the code execute asyncly - it probably reverted to sync further down the chain somewhere.

Right. I only showed up here because a bright red exception showed up in my debug output yesterday after revisiting an old project and upgrading packages. Figuring out it was an invisible call to Dispose in a lib was a treat...

from fluentftp.

Figuring out it was an invisible call to Dispose in a lib was a treat...

Well, although I am really sorry about that, when there was no bright red exception, you just were not aware that the code was running sync when disposing/disconnection. Probably the incurred short block on the QUIT command and GetReply was not to detrimental to be noticed.

from fluentftp.

Nothing to be sorry about, it's all just code. This was just a case of peeling back layers of an onion, which was interesting.

I'm glad we got a temporary fix in place and figured out a good path forward here.

from fluentftp.

| also involves modifying FtpSocketStream to implement...

I see better now what you mean. m_stream.DisposeAsync(). Nifty, didn't know it existed.

from fluentftp.

Gosh, thanks to your efforts I have put together a PR (tested and looks real nice) - would you like to try it and/or look at it (my branch for the PR is https://github.com/FanDjango/FluentFTP/tree/asynctest).

Users who don't want to add the await to their "using" statements, will get the .GetWaiter().GetResult() logic you proposed, also users who don't have one of the more modern .NETs.

from fluentftp.

Per my comment on the PR: #1466 (comment)

We would need something like this:

FtpSocketStream.cs

public override async ValueTask DisposeAsync() {
	// Perform async cleanup.
	await DisposeAsyncCore();

	// Maybe necessary
	await base.DisposeAsync();

	// Maybe necessary
	base.Dispose(false);

	// Suppress finalization.
	GC.SuppressFinalize(this);
}

protected async ValueTask DisposeAsyncCore() {
	if (m_customStream != null) {
		try {
			m_customStream.Dispose();
		}
		catch {
		}

		m_customStream = null;
	}


	if (m_sslStream != null) {
		try {
			await m_sslStream.DisposeAsync();
		}
		catch {
		}

		m_sslStream = null;
	}

	if (m_bufStream != null) {
		try {
			// ensure the last of the buffered bytes are flushed
			// before we close the socket and network stream
			await m_bufStream.FlushAsync();
		}
		catch {
		}

		m_bufStream = null;
	}

	if (m_netStream != null) {
		try {
			await m_netStream.DisposeAsync();
		}
		catch {
		}

		m_netStream = null;
	}

	DisposeSocket();

	if (Client.Status.DaemonRunning) {
		if (!this.IsControlConnection) {
			Client.Status.DaemonCmdMode = true;
		}
	}
}

Where:

• m_customStream: I don't know what this is used for
• m_sslStream: looks to be another stream implementation with some hacky stuff going on that would need to be cleaned up:

• m_bufStream: not custom, no extra work
• m_netStream: not custom, no extra work

from fluentftp.

Let's merge this first part and consider the SocketStream disposal (cleanup) a separate issue for now.

m_customStream: I don't know what this is used for

This is a drop-in placeholder for "other" stream implementations, currently needed and used for those who attach FluentFTP.GnuTls onto FluentFTP. This stream actually does not work async, those who use it thusly will have block issues.

m_sslStream: looks to be another stream implementation with some hacky stuff going on that would need to be cleaned up

Hacky stuff, yes.

Disposal of an encrypted stream entails one or two TCP send operations to send commands to the other end (for orderly shutdown) and these could block. Without these commands, some servers will balk and report errors, thereby failing a transfer.

All other disposal concerns getting rid of buffers and other objects, tasks not likely to hurt you if they are not running async.

Therefore right now, it is my intention to defer the cleanup of SocketStream to a (say) "stage 2".

from fluentftp.

m_sslStream: looks to be another stream implementation with some hacky stuff

The thing about this one is that earlier sslStream implementations (by MicroSoft) did not issue the needed shutdown commands on the encrypted TCP connection prior to disposing. When they finally added that, they only put it into the DisposeAsync that they by then provided in the course of adding this language feature - with a subtle difference in which .NET versions it was included in and which ones not due to overlaps in the development teams schedules.

So getting this absolutely right needs a plethora of additional #IFDEFs etc.

For those versions that needed the hacky "manual" send of the commands, there is also a caveat.

You could have either of these following situations:

1. .NET is very old

You need the hacky manual command send and you cannot use DisposeAsync at all

So if in asyncClient you need .GetAwaiter and .GetResult

2. .NET is in a small range (spare me the effort to say exactly which)

You need the hacky manual commands but you could use DisposeAsync (which we don't do in the current code)

3. .NET is new

You don't need the hacky manual commands and you can use DisposeAsync

from fluentftp.

m_customStream

Here, things are quite clear. I could force implementation of DisposeAsync in the interface and an implementation of CustomStream (currently, only one) could internally go the .GetAwaiter.GetResult way (which the current one, GnuTlsStream, would need to do).

from fluentftp.

FYI I have merged a number of PR's incrementally adding the CloseAsync/DisposeAsync patterns to FtpDataStream and FtpSocketStream into the current master, works as desired now.

Apart from a small caveat concerning closing/disposing SslStream/CustomStream, which may block (very short), all Async client APIs and methods are now diverted to AsyncClose/AsyncDispose instead of as previously, to sync.

Methinks we are finally clean on sync/async code routing.

I would appreciate it if you would take another look to see if you have additional items for this.

from fluentftp.

I think m_bufStream is getting implicitly synchronously disposed since only FlushAsync is called and not DisposeAsync as well.

Although I'm not sure if it matters? I don't know if explicitly calling its Dispose methods does anything at this point or is superfluous (or perhaps this was never being disposed all the way?). Perhaps add, perhaps not:

			if (m_bufStream != null) {
				try {
					// ensure the last of the buffered bytes are flushed
					// before we close the socket and network stream
					await m_bufStream.FlushAsync();
#if NETSTANDARD2_1_OR_GREATER || NET5_0_OR_GREATER
					await m_bufStream.DisposeAsync();
#else
					((IInternalFtpClient)Client).LogStatus(FtpTraceLevel.Verbose, "Disposing(sync) BufferedStream of FtpSocketStream");
					m_bufStream.Dispose(); // Async dispose not supported in this .NET?
#endif
				}
				catch {
				}
				m_bufStream = null;
			}

If this is added here, then a call to m_bufStream.Dispose should also be added to the sync version of FtpSocketStream.Dispose.

from fluentftp.

I just F12'd into it to check and this is how BufferedStream implements its DisposeAsync:

        public override async ValueTask DisposeAsync()
        {
            try
            {
                if (_stream != null)
                {
                    try
                    {
                        await FlushAsync().ConfigureAwait(false);
                    }
                    finally
                    {
                        await _stream.DisposeAsync().ConfigureAwait(false);
                    }
                }
            }
            finally
            {
                _stream = null;
                _buffer = null;
                _writePos = 0;
            }
        }

So we can simplify things to just call DisposeAsync instead of FlushAsync.

Same goes for the sync version:

protected override void Dispose(bool disposing)
{
    try
    {
        if (disposing && _stream != null)
        {
            try
            {
                Flush();
            }
            finally
            {
                _stream.Dispose();
            }
        }
    }
    finally
    {
        _stream = null;
        _buffer = null;
        _writePos = 0; // WriteByte hot path relies on this

        // Call base.Dispose(bool) to cleanup async IO resources
        base.Dispose(disposing);
    }
}

from fluentftp.

I was coding from FtpSocketStream on upwards.

Boring down into the streams..., well, need also to check .NET version by version...

from fluentftp.

I did check version 4.7.2 of Dispose, and it also makes the flush call. I assume whenever DisposeAsync was introduced that it followed this logic.

In any case, by only ever calling Flush I don't think this BufferedStream was ever being cleaned up entirely.

from fluentftp.

In any case, by only ever calling Flush I don't think this BufferedStream was ever being cleaned up entirely.

Thankfully, it wasn't in universal use anyway for newer .NETs.

Why don't you submit a PR with the needed changes?

from fluentftp.

Thanks for the PR, that saved some time. It's merged into the current master and anyone who has been having async glitches should put this through the grinder.

from fluentftp.

Have you had a chance to use current master?

from fluentftp.

Seems to work fine, at least in a basic test app 👍

from fluentftp.