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

Allows for zero bytes to be read #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acragusa
Copy link

@acragusa acragusa commented Mar 8, 2019

As per the original project: #202

I can confirm this addresses a read error on 0 byte payloads.

@oznetmaster
Copy link
Owner

oznetmaster commented Mar 9, 2019

The Crestron documentation clearly states Streams return zero (0) only at the end of the stream,:

    public virtual IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object state);
    /// <summary>
    /// Waits for the pending asynchronous read to complete.
    /// 
    /// </summary>
    /// <param name="asyncResult">The reference to the pending asynchronous request to finish.
    ///             </param>
    /// <returns>
    /// The number of bytes read from the stream, between zero (0) and the number
    ///                  of bytes you requested. Streams return zero (0) only at the end of the stream,
    ///                  otherwise, they should block until at least one byte is available.
    /// 
    /// </returns>
    /// <exception cref="T:System.ArgumentNullException">asyncResult is null.
    ///             </exception><exception cref="T:System.ArgumentException">asyncResult did not originate from a CrestronIO.Stream.BeginRead(System.Byte[],System.Int32,System.Int32,System.AsyncCallback,System.Object)
    ///                  method on the current stream.
    ///             </exception><exception cref="T:Crestron.SimplSharp.CrestronIO.IOException">The stream is closed or an internal error has occurred.
    ///              </exception>
    public virtual int EndRead(IAsyncResult asyncResult);

Your change appears to indicate that the Crestron documentation is not correct?

Looking at the underlying code for BeginRead/Read etc, it is clear that if this works for you, it is an accident.

There are a number of incorrectly implemented methods in the Crestron code which fail if the requested read count is 0. For instance:

public virtual int Read([In, Out] char[] buffer, int index, int count)
{
    if (buffer == null)
    {
        throw new ArgumentNullException();
    }
    if (index < 0)
    {
        throw new ArgumentOutOfRangeException();
    }
    if (count < 0)
    {
        throw new ArgumentOutOfRangeException();
    }
    if ((buffer.Length - index) < count)
    {
        throw new ArgumentException();
    }
    int num = 0;
    while (true)
    {
        int num2 = this.Read();
        if (num2 != -1)
        {
            num++;
            buffer[index + num] = (char) num2;
            if (num < count)
            {
                continue;
            }
        }
        return num;
    }
}

you will note that even if count is 0, if there is data available, the method will return 1, and put one character into the buffer, removing that character from the input data stream.

This same kind of problem is elsewhere in the Crestron code.

Also, the change you are requesting ignores the code above it:

					if (nread == 0 && retry < _retry)
						{
						retry++;
						stream.BeginRead (buff, offset, length, callback, null);

						return;
						}

which will go through multiple retires if nread is 0.

It does appear that code can be written to correctly handle a read request with a length of 0, but it is significantly more extensive then just changing this one statement.

I will investigate this further, and propose changes which will hopefully address this issue correctly.

@oznetmaster
Copy link
Owner

I realize that what I fixed is not what you were asking :(

I did fix a significant infrastructure issue with requesting to read zero bytes, but that is not what your problem is.

I am researching a correct solution to your problem. The one you proposed still has the other issues associated with it.

@oznetmaster
Copy link
Owner

oznetmaster commented Mar 9, 2019

I am still confused as to why this change is necessary. If nread is zero, the operation is retried five times. If nread < length, the incoming data is buffered until the entire requested length is available.

The only way the problem you are trying to address could occur is if nread is zero more then five times without receiving any data at all. That would seem to be a real error condition.

If this is a real, reoccurring, condition, it would seem that either increasing the number of retries, or putting in some amount of random delay between retries, would be the correct approach.

@oznetmaster
Copy link
Owner

In fact, the original post @acragusa referenced was not about allowing for zero bytes to be read, but about "We noticed that 'NetworkStream.EndRead' can return zero (0) bytes even when connection is not being closed. Close reading of documentation of 'EndRead' only says that zero bytes is returned when connection is closed. Other way around is not always true, receiving 0 bytes does not always mean connection is being closed it seems. It is quite common to get zero bytes returned even when the connection is not being closed. Happens once in a while with Chrome, and seems to happen more often with Firefox."

I believe that is why the retry code was added to this method in the first place.

@acragusa, if your problem is something different (0 byte payloads), then the fixes I have made and posted in SSMonoNetLibrary should solve your problem. Please try the new library.

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.

None yet

2 participants