Skip to content

[celluloid] fix call on protected method#49

Merged
dblock merged 1 commit intomasterfrom
celluloid-public-send
Jan 22, 2016
Merged

[celluloid] fix call on protected method#49
dblock merged 1 commit intomasterfrom
celluloid-public-send

Conversation

@mikz
Copy link
Copy Markdown
Contributor

@mikz mikz commented Jan 18, 2016

closes #40

E, [2016-01-18T18:58:44.358004 #48656] ERROR -- : Actor crashed!
NoMethodError: protected method `connected?' called for #<Slack::RealTime::Concurrency::Celluloid::Socket:0x007f8d73c5f3c0>
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `public_send'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `dispatch'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/call/sync.rb:16:in `dispatch'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:50:in `block in dispatch'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:76:in `block in task'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/actor.rb:339:in `block in task'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task.rb:44:in `block in initialize'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task/fibered.rb:14:in `block in create'
[2016-01-18 18:58:44] ERROR NoMethodError: protected method `connected?' called for #<Slack::RealTime::Concurrency::Celluloid::Socket:0x007f8d73c5f3c0>
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `public_send'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `dispatch'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/call/sync.rb:16:in `dispatch'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:50:in `block in dispatch'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:76:in `block in task'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/actor.rb:339:in `block in task'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task.rb:44:in `block in initialize'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task/fibered.rb:14:in `block in create'
    (celluloid):0:in `remote procedure call'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/call/sync.rb:45:in `value'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/proxy/sync.rb:38:in `method_missing'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/slack-ruby-client-0.5.3/lib/slack/real_time/client.rb:56:in `started?'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/slack-ruby-client-0.5.3/lib/slack/real_time/client.rb:114:in `send_json'
    /Users/user/.rvm/gems/ruby-2.2.4/gems/slack-ruby-client-0.5.3/lib/slack/real_time/api/message.rb:15:in `message'

so move the #connected? to public

TODO

  • add a test

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 18, 2016

This was brought up before in https://github.com/dblock/slack-ruby-client/pull/40. I'd like a spec that reproduces this problem, do you know how to make this happen?

@mikz
Copy link
Copy Markdown
Contributor Author

mikz commented Jan 18, 2016

Yep. I have client that produces this, so I will try to write a minimal test case.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 19, 2016

I had tried to reproduce this without success. The integration tests in spec that run with a token could be a good place to start.

@mikz mikz self-assigned this Jan 19, 2016
@mikz mikz force-pushed the celluloid-public-send branch from dfb73b5 to 52bc868 Compare January 22, 2016 12:00
@mikz
Copy link
Copy Markdown
Contributor Author

mikz commented Jan 22, 2016

@dblock done!

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2016

So this has a fix, but CI has failed? So it didn't really fix it?

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2016

Assuming you can figure the CI story out, this needs a CHANGELOG entry, please.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2016

Oh duh it's just Rubocop. Fix the build, add to CHANGELOG and it's good to go. Thanks!

```
E, [2016-01-18T18:58:44.358004 #48656] ERROR -- : Actor crashed!
NoMethodError: protected method `connected?' called for #<Slack::RealTime::Concurrency::Celluloid::Socket:0x007f8d73c5f3c0>
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `public_send'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `dispatch'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/call/sync.rb:16:in `dispatch'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:50:in `block in dispatch'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:76:in `block in task'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/actor.rb:339:in `block in task'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task.rb:44:in `block in initialize'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task/fibered.rb:14:in `block in create'
[2016-01-18 18:58:44] ERROR NoMethodError: protected method `connected?' called for #<Slack::RealTime::Concurrency::Celluloid::Socket:0x007f8d73c5f3c0>
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `public_send'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/calls.rb:28:in `dispatch'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/call/sync.rb:16:in `dispatch'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:50:in `block in dispatch'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/cell.rb:76:in `block in task'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/actor.rb:339:in `block in task'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task.rb:44:in `block in initialize'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/task/fibered.rb:14:in `block in create'
	(celluloid):0:in `remote procedure call'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/call/sync.rb:45:in `value'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/celluloid-0.17.2/lib/celluloid/proxy/sync.rb:38:in `method_missing'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/slack-ruby-client-0.5.3/lib/slack/real_time/client.rb:56:in `started?'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/slack-ruby-client-0.5.3/lib/slack/real_time/client.rb:114:in `send_json'
	/Users/user/.rvm/gems/ruby-2.2.4/gems/slack-ruby-client-0.5.3/lib/slack/real_time/api/message.rb:15:in `message'
```
@mikz mikz force-pushed the celluloid-public-send branch from 52bc868 to cf89021 Compare January 22, 2016 13:34
@mikz
Copy link
Copy Markdown
Contributor Author

mikz commented Jan 22, 2016

Looks like the build fails because it expects messages on the same channel, when running in parallel.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2016

That's an interesting failure. Should probably fix this to just wait for a message that we expect instead of checking that the message received is exactly the message we expect. Can do this later, I'm going to try to kick this build into passing just to confirm something is not permanently broken.

dblock added a commit that referenced this pull request Jan 22, 2016
[celluloid] fix call on protected method
@dblock dblock merged commit 1dfed51 into master Jan 22, 2016
@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2016

Merged, thanks for seeing this through.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2016

dblock@e4bf99a

@mikz mikz deleted the celluloid-public-send branch January 22, 2016 15:02
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.

2 participants