NullReferenceException in ImapClient.Disconnect

[ekalchev]

Occurred in master, commit SHA 2681826

Engine.Stream was null. We have this code

try {
    imapClient.Connect();
} catch(Exception) {
} finally {
    if (this.IsConnected == true) {
        this.Disconnect(true);
    }
}

I suspect Connect failed and Disconnect was called on not connected imapClient and somehow IsConnected was true.

We observed this problem when we had a bug in our software where incorrect security method was used. Our mail server support only SSL on connect but we used incorrectly StartTLS. I suspect mailkit was connected and tried to call STARTTLS on the connected connection but that failed and left the Engine object in invalid state where Stream was null and IsConnected was true.

[jstedfast]

This seems like another case of issue #851

ImapClient.IsConnected is a simple property that returns ImapEngine.IsConnected which, in turn, returns true only if the Stream is non-null and ImapStream.IsConnected also returns true.

As with the case of issue #851, I suspect that the issue is that the server is sending * BYE?

Do you happen to have logs? Is that the case?

If you don't have logs, can you replicate the error?

[ekalchev]

I don't have logs and it is not reproducible.

[Miraichi]

You have name of server maybe?

[ekalchev]

You have name of server maybe?

It is private server - Dovecot.

[jstedfast]

I don't think that the scenario that you suspect happened could happen. If your mail server only supports SSL-on-connect, then these are the possible scenarios:

1. You used Connect (host, port, true) or you used Connect (host, port, SecureSocketOptions.SslOnConnect). If this is the case, MailKit would not attempt STARTTLS.
2. You used Connect (host, port, false) or a non-SslOnConnect SecureSocketOption value in which case MailKit would probably hang because the server, being wrapped in SSL, would be expecting the client to initiate an SSL-handshake while MailKit, expecting the server to send a greeting, would wait for the server to send it a greeting. In other words, both client & server would wait endlessly for the other to send it something.

[jstedfast]

The only scenario that I can see happening here is that somehow the ImapEngine's Stream property got set to null between the call to ImapClient.DIsconnect(true) and the LOGOUT\r\n command being run on the ImapEngine.

Unless I'm missing something (completely possible), I think that another thread must have been accessing the ImapClient and called Disconnect() on it or something.

[ekalchev]

We have dedicated thread that works with ImapClient. I am sure this was not the case.

[jstedfast]

I'm completely out of ideas :-\

[ekalchev]

We've got try catch around Disconnect and this is not an big issue for now. I'll let you know if happen again.

[jstedfast]

Okay, thanks. If you could confirm that it was a Connect(...) that got an exception followed by Disconnect(true), that would be super helpful (or even discover that it was some other sequence of events that led to this).

[jstedfast]

I'm just not seeing how this is even hypothetically possible...

Here's the ImapClient Disconnect logic:

CheckDisposed ();

if (!engine.IsConnected)
	return;

if (quit) {
	try {
		var ic = engine.QueueCommand (cancellationToken, null, "LOGOUT\r\n");
		await engine.RunAsync (ic, doAsync).ConfigureAwait (false);
	} catch (OperationCanceledException) {
	} catch (ImapProtocolException) {
	} catch (ImapCommandException) {
	} catch (IOException) {
	}
}

disconnecting = true;

engine.Disconnect ();

The if (!engine.IsConnected) check expands to if (engine.Stream == null || !engine.Stream.IsConnected)

This tells me that engine.Stream is not null and it is still considered to be connected at this stage.

BUT... by the time await engine.RunAsync (ic, doAsync).ConfigureAwait (false); is called, engine.Stream is null. How could that possibly happen unless another thread disconnected the client? It can't.

[ekalchev]

Okay, thanks. If you could confirm that it was a Connect(...) that got an exception followed by Disconnect(true), that would be super helpful (or even discover that it was some other sequence of events that led to this).

I think the problem is in our code. We have this

Task connectionTask = ConnectAsync(host, port, options, cancellationToken);
Task.WhenAny(connectionTask, cancellationToken.WhenCanceled()).GetAwaiter().GetResult();

public static class TaskExtensions
    {
        public static Task WhenCanceled(this CancellationToken cancellationToken)
        {
            var tcs = new TaskCompletionSource<bool>();
            cancellationToken.Register(s => ((TaskCompletionSource<bool>)s).SetResult(true), tcs);
            return tcs.Task;
        }
    }

followed by

        if (imapClient.IsConnected == true)
        {
                imapClient.Disconnect(true);
        }

Now I can see that this might be the problem. Our logs showed that the user cancelled Connect method and then Disconnect(true) was called.

We had to do this async method call because cancellation of synchronous Connect takes a lot of time to actually release the thread and we can't close the application. I'll be happy if you suggest a way to force Connect to release immediately.

[jstedfast]

I think I recently made improvements to cancellation that would affect cancelling the Connect[Async] methods.

Is this still an issue in the latest code?

[ekalchev]

We had this issue an year ago. I'll test it with latest code

[ekalchev]

Cancellation of connect method is not an issue anymore with the latest code

[jstedfast]

Awesome :-)