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

python: envoy_requests depend on a specific version of protobuf #1504

Closed
wants to merge 1 commit into from

Conversation

crockeo
Copy link
Contributor

@crockeo crockeo commented Jun 3, 2021

Description: This PR introduces a requirement that consumers of envoy-requests must have protobuf==3.17 to prevent crashes. I've been able to consistently reproduce crashes in Python applications on the main branch by:

  1. Making a virtualenv
  2. Building + installing a wheel built from //library/python:envoy_requests_whl on main
  3. Install protobuf==3.14 (previous version before envoy bump) or protobuf==3.16 (current version after envoy bump)
  4. Run the following script
  5. Crashes with
    1. std::bad_optional_access on protobuf==3.14
    2. segfault on protobuf==3.16
import google.protobuf.descriptor
from envoy_requests.common.engine import Engine
Engine.handle()

This succeeds when running protobuf==3.17, however, hence the version I've chosen to require.

I've narrowed this problem down to an indirect import of google.protobuf.pyext._message. Importing that module calls InitProto2MessageModule which does a lot of initialization, but I'm not sure which part causes the problem. I'm still struggling to understand how this all happens, so if anyone has any ideas I'd love to hear them!

Risk Level: Low
Testing: Documented above
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <chillen@lyft.com>
@goaway
Copy link
Contributor

goaway commented Jun 3, 2021

Re-running unit tests since I don't see any output as to the what the failure was in GitHub Actions.

@crockeo
Copy link
Contributor Author

crockeo commented Jun 3, 2021

closing since this isn't the right fix. see #1506 for more details

@crockeo crockeo closed this Jun 3, 2021
@crockeo crockeo deleted the ch/require-protobuf-of-a-specific-version branch June 3, 2021 20:38
goaway pushed a commit that referenced this pull request Jun 8, 2021
Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking.

Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on.

Risk Level: Low
Testing: See the now-closed #1504 for how I've been testing this.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <chillen@lyft.com>
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.

None yet

2 participants