minghuaw / fe2o3-amqp

A rust implementation of the AMQP1.0 protocol based on serde and tokio.
MIT License
64 stars 7 forks source link

`serde_amqp::read::IoReader` unit test fails after rust is updated to 1.80 #270

Closed minghuaw closed 3 months ago

minghuaw commented 3 months ago

The two unit tests that are failing are

  1. read::ioread::tests::test_incomplete_read_const_bytes_after_peek
  2. read::ioread::tests::test_incomplete_read_const_bytes_without_peek

The cause of this is introduced in a change of behaviour in std::io::Cursor (rust-lang/rust#125123) where the cursor would drain (ie. move pos to the end) even if read_exact or read_buf_exact fails.

This is a more general problem than just Cursor as we were relying on unspecified behaviour. There's a warning for the error case in std::io::Read::read_exact

If this function returns an error, it is unspecified how many bytes it has read, but it will never read more than would be necessary to completely fill the buffer.

minghuaw commented 3 months ago

~However, the change in std::io::Cursor should not be a problem for fe2o3-amqp as the IoReader is only used with bytes::Buf::reader(), which would never return an error according to its doc (https://docs.rs/bytes/latest/bytes/buf/trait.Buf.html#method.reader)~

This function returns a new value which implements Read by adapting the Read trait functions to the Buf trait functions. Given that Buf operations are infallible, none of the Read functions will return with Err.

minghuaw commented 3 months ago

This should not be a problem for fe2o3-amqp because the IoReader is used within the frame decoder which is only called after FramedRead with LengthDelimitedCodec, which ensures that the bytes are only returned when a whole buffer is ready.

However, this leaves a few problems

The unit tests should reflect these changes as well

minghuaw commented 3 months ago

~The FrameDecoder probably doesn't need much change other than what would be necessary as a result of the two mentioned above because it is chained with a LengthDelimitedCodec, however, it might be worth considering not consuming all the bytes before the decoding succeeds.~

~- [ ] Do not consume src buffer before decoding succeeds.~

Moving this to a separate issue