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

Allow loading grpc/errors.rb before grpc.rb #19893

Merged
merged 1 commit into from
Aug 13, 2019

Conversation

blowmage
Copy link
Contributor

@blowmage blowmage commented Aug 8, 2019

As you know, the Ruby gem loads the compiled gRPC library and initializes all system resources when the following is called in a Ruby file:

require 'grpc'

Once the compiled gRPC library is loaded the Ruby process cannot be forked, so it is very common for Ruby apps to delay requiring GRPC until right before it is needed. (See #8798) However, it would be very beneficial for Ruby apps to be able to reference some GRPC code structures before the compiled gRPC library is loaded. For instance, it would be beneficial to be able to register error handlers.

This PR makes a couple small changes. First, it removes the require_relative './grpc' from grpc/errors.rb, allowing the error classes to be loaded before the compiled gRPC library. Now the grpc/errors.rb file behaves the same as the grpc/logconfig.rb file, where require 'grpc/logconfig' can be called before require 'grpc' which loads the compiled gRPC library.

Second, it adds the file grpc/status_codes.rb that defines GPRC::StatusCodes to the gem. GPRC::StatusCodes is the same structure as GPRC::Core::StatusCodes, but the latter is defined in the compiled gRPC library, and therefore can't be used. So GPRC::StatusCodes has been added that defines the same values, with documentation, so it can also be documented as part of the public API. The grpc/errors.rb file now depends on this new grpc/status_codes.rb file.

These changes allow errors to be loaded before the compiled gRPC library and is backwards compatible to previous releases.

@apolcyn apolcyn self-assigned this Aug 8, 2019
@apolcyn apolcyn added the release notes: yes Indicates if PR needs to be in release notes label Aug 8, 2019
@apolcyn
Copy link
Contributor

apolcyn commented Aug 8, 2019

I'm thinking we can still save the benefits of this change, but without adding the second clone module.

Is it possible to just move the status code definitions in the C extension to this pure ruby file, but keep all of the constants under the same module?

I.e., I'm thinking we could delete this function: https://github.com/grpc/grpc/blob/master/src/ruby/ext/grpc/rb_grpc.c#L158, and then have the new ruby file put the codes under GRPC::Core

Also, can we add a test script to illustrate this; we could probably have a small test script under https://github.com/grpc/grpc/tree/master/src/ruby/end2end, and add it to the list of test scripts invoked from https://github.com/grpc/grpc/blob/master/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh (these tests are meant to do things that are cleaner ran in their own process).

@blowmage
Copy link
Contributor Author

blowmage commented Aug 9, 2019

I'm thinking we can still save the benefits of this change, but without adding the second clone module.

Funny enough, using StatusCodes before the compiled gRPC library is loaded was my next requested change. We would love to be able to reference those constants.

Is it possible to just move the status code definitions in the C extension to this pure ruby file, but keep all of the constants under the same module?

Yes, that should be possible. I wanted this PR to be as additive as possible, so I didn't want to mess with that existing code. Would you like me to make that change in this PR?

@apolcyn
Copy link
Contributor

apolcyn commented Aug 9, 2019

Yes, that should be possible. I wanted this PR to be as additive as possible, so I didn't want to mess with that existing code. Would you like me to make that change in this PR?

Yes please, I'd prefer to do this one as an atomic change.

@blowmage
Copy link
Contributor Author

@apolcyn I think i fixed the copyright problem. Can you restart Kokoro?

@blowmage
Copy link
Contributor Author

Are those Kokoro failures due to the changes made in this PR?

@blowmage
Copy link
Contributor Author

@apolcyn The tools scripts that failed on Kokoro pass for me locally. Do you understand why it failed? Should I rebase this PR on latest master?

@@ -0,0 +1,135 @@
# Copyright 2015 gRPC authors.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I've fixed the copyright failure, should these new files all use 2019?

@apolcyn
Copy link
Contributor

apolcyn commented Aug 12, 2019

Those errors look unrelated to this change, no need ot rebase

@apolcyn
Copy link
Contributor

apolcyn commented Aug 13, 2019

@blowmage before merging, can you please squash down the commits into one, e.g. with a git rebase ?

@blowmage
Copy link
Contributor Author

Squashed.

@apolcyn
Copy link
Contributor

apolcyn commented Aug 13, 2019

node macos: #19422
objc examples: #19925

@apolcyn
Copy link
Contributor

apolcyn commented Aug 13, 2019

C/C++ macos: #19819

@apolcyn apolcyn merged commit 460ccab into grpc:master Aug 13, 2019
@blowmage
Copy link
Contributor Author

@apolcyn Thanks for accepting! This will be released in 1.24.0, right? It isn't going to make it for the upcoming 1.23.0 release?

@apolcyn
Copy link
Contributor

apolcyn commented Aug 13, 2019

Yes, this will be in the 1.24.0 release. And right, the 1.23.0 branch has been cut (we don't tend to backport larger changes like this).

@blowmage
Copy link
Contributor Author

K, I was playing with this some more and it looks like Struct::Status is still being defined in grpc_c. I will submit another PR to define that class in Ruby, and add more test coverage to the end2end scripts. But before I create that PR I would like to get some feedback on #19931.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/ruby release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants