Initial Gevent Compatibility #14561
Merged
Initial Gevent Compatibility #14561
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@a11r @dgquintas @markdroth |
|
|
|
Closed
|
1 similar comment
|
|
1 similar comment
|
|
1 similar comment
|
dgquintas
reviewed
Mar 14, 2018
| @@ -1,4 +1,4 @@ | |||
| cygrpc.c | |||
| cygrpc.cpp | |||
There was a problem hiding this comment.
Cython only generates .cpp files (You can't tell it to generate cc files)
Also, I think you got dragged into this PR because I touched and then reverted a file you are an OWNER for, but I can't remove you as a reviewer (so you can ignore this pr)
nathanielmanistaatgoogle
suggested changes
Mar 15, 2018
| def init_grpc_gevent(): | ||
| _initialize_grpc_gevent_loop(False) | ||
|
|
||
| def initialize_grpc_gevent_loop_test(): |
There was a problem hiding this comment.
Are we mixing production code and test code? How hard did you look at keeping them separate? Should we file a low-priority cleanup issue documenting that we're mixing production code and test code?
There was a problem hiding this comment.
The test code was a vestige of running the c-core end-to-end tests with gevent.
Because that is not part of this PR, I've removed it for now.
|
|
||
| g_event = gevent_event.Event() | ||
|
|
||
| cdef void init_loop() with gil: |
There was a problem hiding this comment.
The Python subcodebase consistently maintains definition-before-use order (in both Cython and Python). Can that be maintained in this added code?
| @@ -11,6 +11,7 @@ | |||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # distutils: language=c++ | |||
There was a problem hiding this comment.
I don't understand this and it isn't mentioned in the commit log message. What's going on here?
There was a problem hiding this comment.
I've added a comment in the commit message.
Because some c++ from core ends up leaking in certain headers, I needed to change the cython extension to be built with c++.
src/python/grpcio_tests/setup.py
Outdated
| @@ -50,7 +50,8 @@ | |||
| 'build_package_protos': grpc_tools.command.BuildPackageProtos, | |||
| 'build_py': commands.BuildPy, | |||
| 'run_interop': commands.RunInterop, | |||
| 'test_lite': commands.TestLite | |||
| 'test_lite': commands.TestLite, | |||
| 'test_gevent': commands.TestGevent | |||
There was a problem hiding this comment.
Always include the comma after the last element in multiline collection and dictionary literals.
|
Addressed comments and fixed some sanity failures |
|
1 similar comment
|
|
|
|
|
It looks like Eventlet is a separate event loop than gevent, so no this PR is not sufficient to support it. However, since it provides a |
nathanielmanistaatgoogle
suggested changes
Mar 26, 2018
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """gRPC's Python gEvent APIs""" |
There was a problem hiding this comment.
Missing period at the end of this doc string.
| def init_gevent(): | ||
| """Patches gRPC's libraries to be compatible with gevent. | ||
|
|
||
| This must be called AFTER the python standard lib has been patched, |
There was a problem hiding this comment.
This line (and the other two-space-indented lines in this doc string) should be four-spaces-indented.
I think this is one of those things where yapf isn't yet smart enough to do it, but mercifully it is smart enough to avoid undoing it.
| @@ -181,6 +187,13 @@ def try_set_handler(name, handler): | |||
| # Run the tests | |||
| result.startTestRun() | |||
| for augmented_case in augmented_cases: | |||
| skip_test = False | |||
There was a problem hiding this comment.
Move this assignment to an else: attached to the for:.
(Then see if the local field doesn't just evaporate entirely if the continue is also moved into the else:?)
| @@ -237,6 +237,7 @@ def testUpDown(self): | |||
| self.assertIsNotNone(service.servicer_methods) | |||
| self.assertIsNotNone(service.server) | |||
| self.assertIsNotNone(service.stub) | |||
| service.server.stop(None) | |||
There was a problem hiding this comment.
I think the changes to this file and _split_definitions_test are worth breaking out into a separate pull request.
My apologies for not calling it out earlier in review.
There was a problem hiding this comment.
I think there is important context to this change in this PR. I've broken it into a separate commit for some separation though.
.gitignore
Outdated
| @@ -15,7 +15,7 @@ python_pylint_venv/ | |||
| htmlcov/ | |||
| dist/ | |||
| *.egg | |||
| py27/ | |||
| py27*/ | |||
There was a problem hiding this comment.
Ah, okay.
I have an awful feeling that py3[0-9]* was intended to be a regular expression rather than a glob. Knowing now that they are globs, can we rewrite these to only capture what we really intend to capture? It'd be nice in an explicitly self-documenting kind of way if these contained _native and _gevent in them.
|
When will the PR be merged? Thanks |
Relying on garbage collection to stop servers breaks with gevent.
Because some cpp code ends up leaking into cython, we change the cython generator to generate cpp code.
|
|
|
|
@kpayson64 , are you aware that this PR disables python dbg tests entirely for all of linux, mac and windows? I don't think we want that. https://github.com/grpc/grpc/pull/14561/files#diff-38ce68e55e10fc655a0123717db8498dR199 |
Google App Engine Flexible VM. grpc v1.11.0. |
|
@jtattermusch Running with
The Python build system automatically inserts the |
|
@yaalaa |
|
I could try to describe how I've ended up in crash. As to core dump, I'm not sure I can get it from GAE VM.
|
|
@kpayson64 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #4629
This PR needs to be cleaned up and broken into much smaller PRs for review, but should contain the basic components of gevent compatibility.
Adding the above lines should make gRPC work with gevent.