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

Add Customizable Serialization support through Transmog #39

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

Conversation

ecton
Copy link
Member

@ecton ecton commented Jan 5, 2022

This pull request brings in support for Transmog, allowing flexibility in configuring serialization formats. Closes #28.

This is currently a breaking change.

  • Connecting::acceptnow takes a format parameter. For compatibility with existing projects, add transmog-bincode as a dependency, and pass in transmog_bincode::Bincode::default(). The positive side of this breaking change is that it puts the user in full control of their bincode serialization settings. The basic example diff shows the net effect of this breakage.
  • Incoming::accept_with_format/Connection::open_stream_with_format are new functions take a format parameter. This allows using a different serialization format on a per-stream basis, after the r#type phase. The existing functions call through to these new functions passing in the existing format used for r#type.

Things left to do:

  • Consider whether to keep this as a breaking change or to provide a convenience method that continues to use Bincode internally.
  • Update all affected methods' documentation.
  • Optimize sending when serialized_size returns none. Currently does an extra allocation that shouldn't be necessary by pre-padding the byte buffer, serializing, then filling in the length.
  • Add test using Pot -- a format that doesn't support serialized_size currently.

let serialized_length = u64::try_from(serialized_length).expect("not a 64-bit system");
bytes
.get_mut(0..8)
.expect("bytes never allocated")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt really stupid writing this code due to the clippy settings. With this chunk of code, it's crystal clear that this buffer can never be shorter than 8 bytes, yet I'm forced to use the non-panic versions and write expect instead of unwrap().

This code went from readable to a mess quickly, and I feel it's in part due to these clippy settings. I'm providing this as feedback for consideration -- not an edict that we should change the settings.

This commit adds support for specifying the serialization format at the
time of accepting a connection, which means that ALPN protocol
negotiation can be used to control which serialization format is used on
an incoming connection.

The next feature is the ability to switch serialization formats on a
per-stream basis, after the r#type negotation.
@ecton
Copy link
Member Author

ecton commented Jan 5, 2022

@daxpedda This is ready to be looked at -- at least to the extent of answering the outstanding question on breaking the API. Currently this PR causes breaking changes, and I'm not sure what your thoughts are on requiring the user to specify the format themselves moving forward.

I'm going to create a CHANGELOG for Fabruic moving forward, but I wanted to wait to write it until we knew what we were doing with the breaking parts.

@ecton ecton marked this pull request as ready for review January 5, 2022 14:49
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.

Allow sending with other serialization formats
1 participant