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

Sync with an internal fork #96

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Sync with an internal fork #96

wants to merge 17 commits into from

Conversation

nuance
Copy link

@nuance nuance commented Aug 22, 2016

This is broken out into logical commits, tests pass after each commit (so you could choose to accept some subset). Commit messages should be explanatory. I'm happy to help split this apart into a few large logical commits if you'd like to schedule a few hours for some pairing.

Highlights:

  • Ran this on our internal thrift repo (~200 schemas / ~58k loc of thrift), fixed a ton of bugs that came up. Attempted to write test cases for everything I could.
  • Fixed some small runtime bugs, mostly related to error handling (for example, server codecs weren't always clearing their internal buffers etc.)
  • Added more guards against massive structures. Discovered via go-fuzz.
  • Added context.Context support to the generated RPC interfaces (enabled via a flag).

Matt Jones added 17 commits August 24, 2016 17:31
The biggest changes:

- Add boolean constant parsing.
- Add context.Context support to rpc code.

Minor changes encountered while parsing a large thrift corpus:

- Harden code generator against '_test'-suffixed thrift schemas.
- Harden code generator against name collisions ('res').
- Rename internal RPC wrapper structs.

Better error handling:

- Log runtime errors encountered by rpc codec serialization.
- Clear buffers more aggressively.
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.

1 participant