Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

.NET Client - Connection Bugs - Release 2.0.0 #2738

Closed
stuarthunter opened this Issue Dec 4, 2013 · 3 comments

Comments

Projects
None yet
4 participants

Hi,

I have identified two bugs in the .NET Client Connection class (src/Microsoft.AspNet.SignalR.Client/Connection.cs).

1 - Start method will clear required references to _connectTask and _disconnectCts if invoked when connection state != Disconnecting.

The fix is to check the connection state before setting references:

public Task Start(IClientTransport transport)
{
    lock (_startLock)
    {
        if (!ChangeState(ConnectionState.Disconnected, ConnectionState.Connecting))
        {
            return TaskAsyncHelper.Empty;
        }

        _disconnectCts = new CancellationTokenSource();
        _monitor = new HeartbeatMonitor(this, _stateLock);
        _transport = transport;

        _connectTask = Negotiate(transport);
    }

    return _connectTask;
}

2 - Negotiate method will increase value of TransportConnectTimeout every time the connection is started. I believe this should be set to the value returned from the server.

TransportConnectTimeout = TransportConnectTimeout + TimeSpan.FromSeconds(negotiationResponse.TransportConnectTimeout);

should be:

TransportConnectTimeout = TimeSpan.FromSeconds(negotiationResponse.TransportConnectTimeout);

Thanks.

Owner

davidfowl commented Dec 4, 2013

The first one was has already been fixed:
https://github.com/SignalR/SignalR/blob/release/src/Microsoft.AspNet.SignalR.Client/Connection.cs#L409

The second one is a bug. Thanks for the great report.

If you want to fix the latter bug you can follow the steps here https://github.com/SignalR/SignalR/blob/master/CONTRIBUTING.md

@ghost ghost assigned abnanda1 Dec 4, 2013

Owner

davidfowl commented Dec 4, 2013

@abnanda1 take a look at this one. Should be trivial to fix.

@abnanda1 abnanda1 added a commit that referenced this issue Dec 5, 2013

@abnanda1 abnanda1 Modified TransportConnectTimeout so that it doesn't add-up over multi…
…ple negotiate requests

#2738
7c98ace

@ghost ghost assigned gustavo-armenta Dec 11, 2013

Contributor

gustavo-armenta commented Dec 11, 2013

test automation provides coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment