Fix inconsistent behavior for IByteBuffer.Read/WriteBytes(Memory<byte>/Span<byte>) #55

yyjdelete · 2021-05-31T04:39:54Z

No description provided.

…>/Span<byte>)

yyjdelete · 2021-05-31T04:46:30Z

@cuteant
之前测试#51的过程中发现的
WriteBytes写入初始Capacity为0的Buffer会报错,
ReadBytes没有考虑WriterIndex和SlicedByteBuffer的边界
具体的也可以参见我新加的那个测试

但我也不确定现在修改之后的逻辑到底是不是预期的结果

cuteant · 2021-05-31T16:08:16Z

@yyjdelete 我测试下，这块代码有些绕......

cuteant · 2021-05-31T17:08:36Z

src/DotNetty.Buffers/AbstractByteBuffer.NetStandard.cs

            _ = SetBytes(writerIdx, src);
            _writerIndex = writerIdx + src.Length;
            return this;
        }
        public virtual IByteBuffer WriteBytes(in ReadOnlyMemory<byte> src)
        {
            var writerIdx = _writerIndex;
+            EnsureWritable0(writerIdx, src.Length);
            _ = SetBytes(writerIdx, src);


这块保留，EnsureWritable0是不能放在 SetBytes方法中

两个WriteBytes的修改都是正确的

cuteant · 2021-05-31T17:15:09Z

src/DotNetty.Buffers/CompositeByteBuffer.NetStandard.cs

@@ -187,6 +187,44 @@ protected internal override Span<byte> _GetSpan(int index, int count)
            }
        }

+        protected internal override void _GetBytes(int index, Span<byte> destination, int length)


要增加_GetBytes这种方法，要扩展的类还有好多，而且功能与_GetReadableSpan和_GetReadableMemory 两个方法重复，我建议删除这些修改，只修改两个AbstractByteBuffer.ReadBytes方法，代码如下：

public virtual int ReadBytes(Span<byte> destination) { var readerIndex = _readerIndex; var readableBytes = Math.Min(_writerIndex - readerIndex, destination.Length); var count = GetBytes(readerIndex, (0u >= (uint)(readableBytes - destination.Length)) ? destination : destination.Slice(0, readableBytes)); if (count > 0) { _readerIndex += count; } return count; } public virtual int ReadBytes(Memory<byte> destination) { var readerIndex = _readerIndex; var readableBytes = Math.Min(_writerIndex - readerIndex, destination.Length); var count = GetBytes(readerIndex, (0u >= (uint)(readableBytes - destination.Length)) ? destination : destination.Slice(0, readableBytes)); if (count > 0) { _readerIndex += count; } return count; }

也就是说，只用修改这 AbstractByteBuffer的ReadBytes、SetBytes、WriteBytes就可以通过你新增的单元测试了

我之前想过, 但这样

除非重写所有的SlicedByteBuffer的GetBytes实现, 不然无法保证GetBytes读取的长度会被限制在Slice自身区段内, 而不会泄漏区段外的数据.(好像忘了加测试了)

另外CompositeByteBuffer的GetReadableMemory对于内部有多个buffer的情况不是零拷贝的, 可能还是需要重写方法并使用GetSequence()+System.Buffers.BuffersExtensions.CopyTo<T>(this in ReadOnlySequence<T> source, Span<T> destination)代替(或者直接基类就用GetSequence

上面的长度判断有问题，不应该只判断与readablebytes相等的

0u >= (uint)xxx ?
这种好像在其他代码里还有很多

其他地方应该没啥问题，用到 0U >= (uint)的地方，校验的数值都是要确保值为非负数（>=0）的

我之前想过, 但这样

除非重写所有的SlicedByteBuffer的GetBytes实现, 不然无法保证GetBytes读取的长度会被限制在Slice自身区段内, 而不会泄漏区段外的数据.(好像忘了加测试了)

另外CompositeByteBuffer的GetReadableMemory对于内部有多个buffer的情况不是零拷贝的, 可能还是需要重写方法并使用GetSequence()+System.Buffers.BuffersExtensions.CopyTo<T>(this in ReadOnlySequence<T> source, Span<T> destination)代替(或者直接基类就用GetSequence

我之前用手机浏览的，没注意到这条信息，我再想下

我想了下，当初设计是有些欠妥，之前感觉Span和Memory.Slice用着很方便，
GetBytes(int, Span),ReadBytes(int, Span)就放弃显示指定读取长度，而是通过返回值来获取读取长度，所以这几个方法中都放弃使用 CheckReadableBytes 做校验，这样看，逻辑都乱套了，你那样改是对的

我之前想过, 但这样

除非重写所有的SlicedByteBuffer的GetBytes实现, 不然无法保证GetBytes读取的长度会被限制在Slice自身区段内, 而不会泄漏区段外的数据.(好像忘了加测试了)

另外CompositeByteBuffer的GetReadableMemory对于内部有多个buffer的情况不是零拷贝的, 可能还是需要重写方法并使用GetSequence()+System.Buffers.BuffersExtensions.CopyTo<T>(this in ReadOnlySequence<T> source, Span<T> destination)代替(或者直接基类就用GetSequence

这儿第二点的情况是要在GetSize中使用的时候单独判断 😄

yyjdelete · 2021-06-01T06:00:39Z

src/DotNetty.Buffers/AbstractByteBuffer.NetStandard.cs

        public virtual IByteBuffer WriteBytes(in ReadOnlySpan<byte> src)
        {
            var writerIdx = _writerIndex;
+            EnsureWritable0(writerIdx, src.Length);
            _ = SetBytes(writerIdx, src);
            _writerIndex = writerIdx + src.Length;
            return this;
        }
        public virtual IByteBuffer WriteBytes(in ReadOnlyMemory<byte> src)
        {
            var writerIdx = _writerIndex;
+            EnsureWritable0(writerIdx, src.Length);
            _ = SetBytes(writerIdx, src);
            _writerIndex = writerIdx + src.Length;
            return this;


所有Memory的实现可以考虑直接调用Span的实现来减少重复代码(但因为virtual无法内联, 会存在相应的开销, 但应该在能接受的范围内)
public virtual IByteBuffer WriteBytes(in ReadOnlyMemory src)
=> WriteBytes(src.Span);

yyjdelete · 2021-06-01T10:19:24Z

src/DotNetty.Buffers/CompositeByteBuffer.NetStandard.cs

@@ -187,6 +187,44 @@ protected internal override Span<byte> _GetSpan(int index, int count)
            }


对于CompositeByteBuffer._GetMemory/_GetSpan, 如果ToComponentIndex(index) == ToComponentIndex(index + count - 1)(即需要获取的内存完整的属于同一区块), 也应该考虑返回数据

对的，当时完全是图省事 👍 😃

cuteant · 2021-06-01T13:28:35Z

SpanNetty/src/DotNetty.Buffers/AbstractByteBuffer.NetStandard.cs

Line 64 in fdcffd8

CheckIndex(index, count);

SpanNetty/src/DotNetty.Buffers/AbstractByteBuffer.NetStandard.cs

Line 82 in fdcffd8

CheckIndex(index, count);

这两个地方也要做 CheckReadableBytes 校验才对

cuteant

后续还会有提交吗？

yyjdelete · 2021-06-02T14:30:23Z

后续还会有提交吗？

在弄了, 之前跑去看tls去了

cuteant · 2021-06-02T15:27:54Z

这儿修改的ReadBytes/GetBytes要不要也显示指定 readcount，与这儿：

SpanNetty/src/DotNetty.Buffers/AbstractByteBuffer.cs

Lines 451 to 455 in fdcffd8

    
           public abstract IByteBuffer GetBytes(int index, byte[] destination, int dstIndex, int length); 
        
           public abstract IByteBuffer GetBytes(int index, IByteBuffer destination, int dstIndex, int length); 
        
           public abstract IByteBuffer GetBytes(int index, Stream destination, int length);

SpanNetty/src/DotNetty.Buffers/AbstractByteBuffer.cs

Lines 920 to 937 in fdcffd8

    
           public virtual IByteBuffer ReadBytes(IByteBuffer dst, int length) 
        
           { 
        
               if (CheckBounds) { CheckWritableBounds(dst, length); } 
        
               _ = ReadBytes(dst, dst.WriterIndex, length); 
        
               _ = dst.SetWriterIndex(dst.WriterIndex + length); 
        
               return this; 
        
           } 
        
           public virtual IByteBuffer ReadBytes(IByteBuffer dst, int dstIndex, int length) 
        
           { 
        
               CheckReadableBytes(length); 
        
               _ = GetBytes(_readerIndex, dst, dstIndex, length); 
        
               _readerIndex += length; 
        
               return this; 
        
           } 
        
           public virtual IByteBuffer ReadBytes(Stream destination, int length)

逻辑同一起来，然后下面两个方法

SpanNetty/src/DotNetty.Buffers/IByteBuffer.NetStandard.cs

Line 34 in fdcffd8

ReadOnlyMemory<byte> GetReadableMemory(int index, int count);

SpanNetty/src/DotNetty.Buffers/IByteBuffer.NetStandard.cs

Line 38 in fdcffd8

ReadOnlySpan<byte> GetReadableSpan(int index, int count);

就可以移除，用扩展方法来实现（调用GetBytes(int index, Span destSpan, int count)）

cuteant · 2021-06-02T16:03:07Z

真是搞晕了去，这几个方法

SpanNetty/src/DotNetty.Buffers/IByteBuffer.NetStandard.cs

Line 34 in fdcffd8

ReadOnlyMemory<byte> GetReadableMemory(int index, int count);

SpanNetty/src/DotNetty.Buffers/IByteBuffer.NetStandard.cs

Line 38 in fdcffd8

ReadOnlySpan<byte> GetReadableSpan(int index, int count);

不能删除的，可以实现零拷贝嘛 😊

yyjdelete · 2021-06-02T16:23:13Z

CompositeByteBuf在可能的情况下尽可能的零拷贝了, 另外补上了之前遗漏的FixedCompositeByteBuf, 相应的GetIoBuffer也做了相应调整;
Memory的方法暂时没有移到扩展函数里面, 如果移动的话, 源码是兼容的, 但会破坏二进制兼容性, 不过估计使用/重写这个方法的人也不多, 要移的话可以再@我下
GetReadableMemory/GetReadableSpan我觉得的确不需要检查CheckReadableBytes啊, 这个风格和其他的GetXxx函数一样, 手动指定了index的都是到Capacity, 而忽略掉WriterIndex的检查;
而GetReadableSpan和GetSpan的主要区别是, 在CompositeByteBuf中, 不能实现零拷贝时, 前者创建新数组复制后返回(好像可以看做GetIoBuffer的Span版本), 而后者抛出异常
GetReadableMemory/GetReadableSpan可能可以通过GetSequence来实现?(未测试, 可以参考现在CompositeByteBuffer的逻辑)

cuteant · 2021-06-02T16:49:53Z

CompositeByteBuf在可能的情况下尽可能的零拷贝了, 另外补上了之前遗漏的FixedCompositeByteBuf, 相应的GetIoBuffer也做了相应调整;

Memory的方法暂时没有移到扩展函数里面, 如果移动的话, 源码是兼容的, 但会破坏二进制兼容性, 不过估计使用/重写这个方法的人也不多, 要移的话可以再@我下

GetReadableMemory/GetReadableSpan我觉得的确不需要检查CheckReadableBytes啊, 这个风格和其他的GetXxx函数一样, 手动指定了index的都是到Capacity, 而忽略掉WriterIndex的检查;
而GetReadableSpan和GetSpan的主要区别是, 在CompositeByteBuf中, 不能实现零拷贝时, 前者创建新数组复制后返回(好像可以看做GetIoBuffer的Span版本), 而后者抛出异常

GetReadableMemory/GetReadableSpan可能可以通过GetSequence来实现?(未测试, 可以参考现在CompositeByteBuffer的逻辑)

前三没问题，第四个我觉得也没必要改了，不同的技术实现，看具体业务具体处理，不是还有 ByteBufferReader 嘛

SpanNetty/src/DotNetty.Buffers/Reader/ByteBufferReader.cs

Line 37 in fdcffd8

public ref partial struct ByteBufferReader

yyjdelete · 2021-06-03T00:11:44Z

如果不需要把Get/Read/Write/SetBytes(...Memory)改成扩展函数的话, 那就改完了

cuteant

嗯，就这样，先合并

yyjdelete added 2 commits May 31, 2021 12:38

Add test for IByteBuffer.Read/WriteBytes(Memory<byte>/Span<byte>)

8ae144a

Fix inconsistent behavior for IByteBuffer.Read/WriteBytes(Memory<byte…

12e5af4

…>/Span<byte>)

yyjdelete requested a review from cuteant May 31, 2021 04:39

cuteant reviewed May 31, 2021

View reviewed changes

cuteant requested changes May 31, 2021

View reviewed changes

yyjdelete commented Jun 1, 2021

View reviewed changes

cuteant reviewed Jun 2, 2021

View reviewed changes

Address PR feedbacks

849cd3f

yyjdelete force-pushed the buf_span branch from 610046b to 849cd3f Compare June 2, 2021 16:26

Add missing tests

6081bdc

cuteant approved these changes Jun 3, 2021

View reviewed changes

cuteant merged commit 20c7bc0 into cuteant:main Jun 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inconsistent behavior for IByteBuffer.Read/WriteBytes(Memory<byte>/Span<byte>) #55

Fix inconsistent behavior for IByteBuffer.Read/WriteBytes(Memory<byte>/Span<byte>) #55

yyjdelete commented May 31, 2021

yyjdelete commented May 31, 2021

cuteant commented May 31, 2021

cuteant May 31, 2021

cuteant May 31, 2021

cuteant May 31, 2021 •

edited

Loading

yyjdelete Jun 1, 2021

cuteant Jun 1, 2021

yyjdelete Jun 1, 2021

cuteant Jun 1, 2021

cuteant Jun 1, 2021

cuteant Jun 1, 2021

cuteant Jun 1, 2021

yyjdelete Jun 1, 2021 •

edited

Loading

cuteant Jun 1, 2021

yyjdelete Jun 1, 2021

cuteant Jun 1, 2021

cuteant commented Jun 1, 2021

cuteant left a comment

yyjdelete commented Jun 2, 2021

cuteant commented Jun 2, 2021 •

edited

Loading

cuteant commented Jun 2, 2021

yyjdelete commented Jun 2, 2021 •

edited

Loading

cuteant commented Jun 2, 2021

yyjdelete commented Jun 3, 2021

cuteant left a comment

		@@ -187,6 +187,44 @@ protected internal override Span<byte> _GetSpan(int index, int count)
		}

Fix inconsistent behavior for IByteBuffer.Read/WriteBytes(Memory<byte>/Span<byte>) #55

Fix inconsistent behavior for IByteBuffer.Read/WriteBytes(Memory<byte>/Span<byte>) #55

Conversation

yyjdelete commented May 31, 2021

yyjdelete commented May 31, 2021

cuteant commented May 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuteant May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyjdelete Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuteant commented Jun 1, 2021

cuteant left a comment

Choose a reason for hiding this comment

yyjdelete commented Jun 2, 2021

cuteant commented Jun 2, 2021 • edited Loading

cuteant commented Jun 2, 2021

yyjdelete commented Jun 2, 2021 • edited Loading

cuteant commented Jun 2, 2021

yyjdelete commented Jun 3, 2021

cuteant left a comment

Choose a reason for hiding this comment

cuteant May 31, 2021 •

edited

Loading

yyjdelete Jun 1, 2021 •

edited

Loading

cuteant commented Jun 2, 2021 •

edited

Loading

yyjdelete commented Jun 2, 2021 •

edited

Loading