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

Including a separate ruby file with a proto definition that has a recursive field results in RuntimeError: can't modify frozen SystemStackError #425

Closed
bufdev opened this issue May 26, 2015 · 12 comments

Comments

@bufdev
Copy link

bufdev commented May 26, 2015

Maybe I'm missing something.

janus.proto imports jet.proto.

If I have two generated protos, jet.rb and janus.rb, in different packages:

After importing jet.rb, if I then import jet.rb, I will get:

2.2.2 :001 > require 'janus'
RuntimeError: can't modify frozen SystemStackError
    from /Users/peter/.rvm/gems/ruby-2.2.2/gems/janus-0.1.0/_ext/ruby/lib/janus/janus.rb:6:in `finalize_to_pool'
    from /Users/peter/.rvm/gems/ruby-2.2.2/gems/janus-0.1.0/_ext/ruby/lib/janus/janus.rb:6:in `build'
    from /Users/peter/.rvm/gems/ruby-2.2.2/gems/janus-0.1.0/_ext/ruby/lib/janus/janus.rb:6:in `<top (required)>'

This actually seems correct, because what I think is happening is there is a global pool that is then finalized.

  1. If I do this:
JANUS_POOL = Google::Protobuf::DescriptorPool.new
JANUS_POOL.build do

And then reference `JANUS_POOL instead:

  Project = JANUS_POOL.lookup("janus.Project").msgclass

Which is not generated code obviously...this doesn't work because the pools don't know about each other (is there a way to merge pools?)

This seems to be a big problem for anyone who has protos that import each other. Am I missing something?

@ngauthier
Copy link

Have you listed jet as a gem dependency of janus?

@bufdev
Copy link
Author

bufdev commented May 26, 2015

Yes

@bufdev
Copy link
Author

bufdev commented May 26, 2015

I actually tried both:

  1. Having jet be a separate gem, and then including it, both with fully qualified paths and not
  2. Compiling jet into janus but in a separate file.

It's a core global state issue

@cfallin
Copy link
Contributor

cfallin commented May 26, 2015

A few things:

  • This case should "just work". I'll look into it.
  • There is a single global pool (Google::Protobuf::DescriptorPool.generated_pool), and this is intentional: it's how a proto defined in one place finds a proto in another.
  • Finalization is per-message, not per-pool. The build block finalizes everything that was defined within it but the intent, certainly, is that many things can be finalized at different times.

I'll figure out what's going on.

@bufdev
Copy link
Author

bufdev commented May 26, 2015

I'm 99% sure I'm just messing it up, but happy to give concrete reproduce
steps

On Tuesday, May 26, 2015, Chris Fallin notifications@github.com wrote:

A few things:

  • This case should "just work". I'll look into it.
  • There is a single global pool (
    Google::Protobuf::DescriptorPool.generated_pool), and this is
    intentional: it's how a proto defined in one place finds a proto in another.
  • Finalization is per-message, not per-pool. The build block finalizes
    everything that was defined within it but the intent, certainly, is that
    many things can be finalized at different times.

I'll figure out what's going on.


Reply to this email directly or view it on GitHub
#425 (comment).

@cfallin
Copy link
Contributor

cfallin commented May 26, 2015

Cool, if you could share your specific build steps and toplevel Ruby that's require-ing the generated code, that would help. I'll see if I can reproduce it in the meantime. Thanks!

@cfallin
Copy link
Contributor

cfallin commented May 26, 2015

I just verified I can require one generated .rb file that requires another generated .rb file, with dependent message types.

Something more subtle must be going on. Are you able to share the contents of your .proto files? Is SystemStackError one of your types? Do you by any chance have a duplicate name, or are trying to modify the definition after the fact somehow?

@bufdev
Copy link
Author

bufdev commented May 26, 2015

let me get something together and I'll send it on, it's probably me :)

On Tuesday, May 26, 2015, Chris Fallin notifications@github.com wrote:

I just verified I can require one generated .rb file that requires
another generated .rb file, with dependent message types.

Something more subtle must be going on. Are you able to share the contents
of your .proto files? Is SystemStackError one of your types? Do you by
any chance have a duplicate name, or are trying to modify the definition
after the fact somehow?


Reply to this email directly or view it on GitHub
#425 (comment).

@bufdev bufdev changed the title No way to include multiple generated ruby files Including a separate ruby file with a proto definition that has a recursive field results in SystemStackError Jun 3, 2015
@bufdev
Copy link
Author

bufdev commented Jun 3, 2015

OK @cfallin I figured out what the issue is, and I put this together to reproduce: https://github.com/peter-edge/ruby-protobuf-bug

If you have another file with a message that has a recursive field, it will error.

Just clone that and type make to see it. Hope this helps.

@bufdev bufdev changed the title Including a separate ruby file with a proto definition that has a recursive field results in SystemStackError Including a separate ruby file with a proto definition that has a recursive field results in RuntimeError: can't modify frozen SystemStackError Jun 3, 2015
@cfallin
Copy link
Contributor

cfallin commented Jun 3, 2015

Thanks. @haberman will be handling the Ruby work from now on -- Josh, could you take a look?

@haberman
Copy link
Member

Thanks for the report! I was able to reproduce this error and reduce it to the following test case:

#!/usr/bin/ruby

require 'google/protobuf'

Google::Protobuf::DescriptorPool.generated_pool.build do
  add_message "A" do
    optional :a, :message, 1, "A"
  end
end

Google::Protobuf::DescriptorPool.generated_pool.build do
end

Running this program yields:

test.rb:11: stack level too deep (SystemStackError)

This is a stack overflow in upb. C backtrace is:

#0  0x00007ffff5521f65 in upb_strtable_lookup2 (t=t@entry=0x7fffffffd790, key=key@entry=0xb5ece0 "A", len=1, v=v@entry=0x7fffff7ff040) at upb.c:4052
#1  0x00007ffff552551a in upb_strtable_lookup (v=0x7fffff7ff040, key=0xb5ece0 "A", t=0x7fffffffd790) at upb.h:804
#2  upb_resolve_dfs (def=0xb5e7c0, addtab=addtab@entry=0x7fffffffd790, new_owner=new_owner@entry=0xb52800, seen=seen@entry=0x7fffffffd7b0, s=s@entry=0x7fffffffd850)
    at upb.c:3483
#3  0x00007ffff552558a in upb_resolve_dfs (def=0xb5e7c0, addtab=addtab@entry=0x7fffffffd790, new_owner=new_owner@entry=0xb52800, seen=seen@entry=0x7fffffffd7b0, 
    s=s@entry=0x7fffffffd850) at upb.c:3497
#4  0x00007ffff552558a in upb_resolve_dfs (def=0xb5e7c0, addtab=addtab@entry=0x7fffffffd790, new_owner=new_owner@entry=0xb52800, seen=seen@entry=0x7fffffffd7b0, 
    s=s@entry=0x7fffffffd850) at upb.c:3497
#5  0x00007ffff552558a in upb_resolve_dfs (def=0xb5e7c0, addtab=addtab@entry=0x7fffffffd790, new_owner=new_owner@entry=0xb52800, seen=seen@entry=0x7fffffffd7b0, 
    s=s@entry=0x7fffffffd850) at upb.c:3497
#6  0x00007ffff552558a in upb_resolve_dfs (def=0xb5e7c0, addtab=addtab@entry=0x7fffffffd790, new_owner=new_owner@entry=0xb52800, seen=seen@entry=0x7fffffffd7b0, 
    s=s@entry=0x7fffffffd850) at upb.c:3497

(etc)

I'll work on a fix.

@haberman
Copy link
Member

Fixed in #530

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

No branches or pull requests

4 participants