Skip to content

draft: implemented alternative protobuf implementation#274

Closed
tmk241 wants to merge 2 commits intoactix:mainfrom
tmk241:alternative-protobuf-implementation
Closed

draft: implemented alternative protobuf implementation#274
tmk241 wants to merge 2 commits intoactix:mainfrom
tmk241:alternative-protobuf-implementation

Conversation

@tmk241
Copy link

@tmk241 tmk241 commented Aug 8, 2022

PR Type

Refactor

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the nightly rustfmt (cargo +nightly fmt).

Overview

Generally I'd like to discuss whether a ProtoBuf implementation for actix should be closer to the existing JSON implementation.
Original JSON implementation

For that reason I created this alternative ProtoBuf implementation.

  • Introduced compression
  • Extended ProtoConfig by allowing the definition of custom content-types & error-handler
  • Ported over most testcases from JSON implementation
  • Aligned code with preexisting JSON implementation
  • Not using LocalBoxFuture anymore

Since I'm rather new to programming in Rust I'd definitely love to hear your inputs on this.
I guess there might be some pitfalls I'm unaware of! 😄

Thanks for having a look

@robjtede robjtede added A-protobuf Project: actix-protobuf B-semver-major breaking change requiring a major version bump labels Aug 9, 2022
@tmk241 tmk241 force-pushed the alternative-protobuf-implementation branch from 43efb3f to 1780f7e Compare August 14, 2022 11:02
@tmk241 tmk241 marked this pull request as ready for review August 14, 2022 11:02
@tmk241 tmk241 force-pushed the alternative-protobuf-implementation branch from d660ebf to f13a001 Compare August 14, 2022 11:05
let res = ready!(Pin::new(&mut *payload).poll_next(cx));
match res {
Some(chunk) => {
let chunk = chunk.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I guesss this unwrap could be risky.

Body {
limit: usize,
length: Option<usize>,
#[cfg(feature = "__compress")]
Copy link
Member

Choose a reason for hiding this comment

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

where does this come from?

@JohnTitor
Copy link
Member

Sorry for the late review. I think the current rewrite isn't production-ready and needs some major follow-ups. Since this patch is now outdated, I'd like to see the latest form of the patch rebased onto the main.
If you're still interested in this, please re-submit a PR.
Thank you for writing and proposing this change!

@JohnTitor JohnTitor closed this Feb 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-protobuf Project: actix-protobuf B-semver-major breaking change requiring a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants