Closed vvvviiv closed 3 months ago
I think we already de-facto have this requirement, if it's not already specified.
Oh, I just found the BufMut::chunk_mut
already had the restrictions.
https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_mut.rs#L166-L179
I will open a PR to fix this issue.
This issue could also be a sub-issue of #178.
According to the docs of
Buf::chunk
:https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L142-L150
It only requires the implementer to return an empty slice when
Buf::remaining
returns 0, but a non-continuous implementation ofBuf::chunk
can return an empty slice even ifBuf::remaining
> 0.For example:
The problem is when you call some methods like
Buf::copy_to_slice
orBuf::copy_to_bytes
, it will cause an infinite loop.Let's see the default implementations of
Buf::copy_to_slice
:https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L268-L282
The
let src = self.chunk();
causes the problem, and its docs does not remind the implementer to override its behavior:https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L247-L268
The
Buf::copy_to_bytes
has the same problem.As for some methods like
Buf::get_u8
, they panic:https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L300-L307
let ret = self.chunk()[0];
cause panic, and its docs only says: "This function panics if there is no more remaining data inself
.":https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L284-L300
But it also panics when
Buf::remaining
> 0.In fact, the problem is already mentioned in https://github.com/tokio-rs/bytes/issues/178#issuecomment-458213222.
These are all because the
Buf::chunk
can return an empty slice even ifBuf::remaining
> 0.Solutions
A quick fix is restricting the
Buf::chunk
to return an empty slice if and only ifBuf::remaining
returns 0, but it also changes the semantics ofBuf::chunk
, should it be considered a breaking change? I can open a PR if the change is acceptable.Any more ideas?