wcharczuk / nsqnet Goto Github PK

Hi,

I've noticed under high load RecieveMessage() does not always read the entire message from the network stream (probably in cases where the stream is not ready to be read)

You should be able to overcome this by changing

var size = BitConverter.ToInt32(sizebuffer, 0);

if (size != 0)
{
    byte[] buffer = new Byte[size];
    _networkStream.Read(buffer, 0, (int)size);

    return UnpackMessage(size, buffer);
}

to:

var size = BitConverter.ToInt32(sizebuffer, 0);

if (size != 0)
{
    byte[] buffer = new Byte[size];
    var read = 0;
    while (read < size) {
        var readCount = _networkStream.Read(buffer, read, (int)size-read);
        if (readCount < 0)
            continue;
         if (readCount == 0)
             break;
         read += readCount;
    }
    return UnpackMessage(size, buffer);
}

I'm working on some changes that I plan to submit a pull request for, which will add support for a number of new NSQ protocol options as well as negotiation for TLS/Deflate/Snappy support, and have a couple questions.

1. Of these changes, the addition of Snappy may be most controversial, since it introduces dependencies on a native library & P/Invoke. I'm using https://www.nuget.org/packages/Snappy.NET/ which includes support for 32 & 64 bit environments. Any concerns/issues with adding this new reference (and the downstream dependency of https://www.nuget.org/packages/Crc32C.NET/)?
2. Are there any strong versioning / compatibility guarantees for NSQClient? I'd like to introduce a NSQOptions class to house the more detailed configuration information, and am wondering how much I can/should clean up existing constructor overloads, or if I should preserve those (as well as passthrough getters/setters on the ShortIdentifier/LongIdentifier/HeartbeatMilliseconds properties, which would be duplicating the underlying options for the client). Given that NSQ has deprecated the ShortIdentifier & LongIdentifier names, a wholesale cleanup/update to match the current protocol state would be cleaner, but would be a breaking change. Does it make sense to roll a new major version for this, or would you prefer to have it implemented as a minor version with compatibility (marking the existing constructors/properties obsolete)? Taking advantage of the new options will require changes for anyone using NSQClient already, and the 'breaking changes' would be relatively minor...
3. A potentially less problematic breaking change - any objection to marking the default NSQClient constructor as private instead of public (regardless of the major/minor versioning decisions)? As far as I can tell, that constructor is pointless, as there's no way to configure the nsqd hostname/port after construction (the NSQClient Hostname/Port properties are read-only) - which means the client should fail upon Initialize(). It seems making that constructor private would make usage more intuitive.

Thanks,
Levi

The calls to WriteAscii followed by WriteBinary appear to be intended to be part of an atomic operation, but are each locked independently. Therefore, the WriteAscii/WriteBinary calls of a pair of concurrent Publish invocations might be interleaved.

https://github.com/politician/NSQnet/blob/master/NSQnet/NSQProtocol.cs#L225-L226

I found in the NSQ client list that the client does not support the TLS protocol, which is very important in some environments at present. Can you extend the support?

Hi,

Is there a way to tell which topic a message came from when subscribing to multiple topics?

Thanks.

In NSQSubscriber.OnMessageReceived, the event handler NSQMessageReceived is read twice outside of a lock, once in the if statement and once in the invocation.

https://github.com/politician/NSQnet/blob/master/NSQnet/NSQSubscriber.cs#L160-L161

Instead, that should be:

var handler = this.NSQMessageReceived;
if (handler != null) {
   handler(this, e);
}