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

Wrap jabber notifications in a timeout block #929

Closed
wants to merge 71 commits into from
Closed

Wrap jabber notifications in a timeout block #929

wants to merge 71 commits into from

Conversation

@funglaub
Copy link
Contributor

@funglaub funglaub commented Aug 28, 2015

We had a lot of trouble recently with xmpp4r/xmpp4r#23, passenger reports lots of requests in the queue and all workers were dead (not responding) causing errors not being reported to errbit.

I isolated the issue and it looks like the xmpp4r deadlock is responsible for this. I wrapped all those calls into a simple Timeout#timeout block and now they are gone.

@stevecrozz
Copy link
Member

@stevecrozz stevecrozz commented Sep 3, 2015

Timeout does worry me a bit, see http://www.mikeperham.com/2015/05/08/timeout-rubys-most-dangerous-api/ It sounds like some kind of underlying IO resource is should have a timeout set, but doesn't, and applying a timeout using Timeout could end up causing other problems when we start running multiple errbit threads and sharing connections.

I could still be persuaded to add this, though, because I'm planning to move all the notifier concerns into external Gems. We could fast-track that effort a bit if you're willing to help, especially if you want to also maintain the not-yet-existing errbit_gtalk_plugin.

@funglaub
Copy link
Contributor Author

@funglaub funglaub commented Sep 11, 2015

I'm aware that Timeout::timeout might be dangerous, but xmpp4r/xmpp4r#23 does cause de facto problems in our setup. I agree with you that Timeout might not be the best solution in this matter, but I don't see any other options right now to deal with the issue.

I'd happy to support your efforts in refactoring the whole notificatier concerns ;)

stevecrozz and others added 9 commits Sep 11, 2015
@stevecrozz
Copy link
Member

@stevecrozz stevecrozz commented Sep 11, 2015

@funglaub Would you like to connect on https://gitter.im/errbit/errbit and see if we can come up with a plan?

rud and others added 25 commits Oct 19, 2015
That's what the majority of the codebase is using, so following that
Notice the return value of a before_action is ignored, so we can safely remove that code.
Conflicts:
	.rubocop.yml
	.rubocop_todo.yml
	Gemfile
	app/helpers/apps_helper.rb
	app/helpers/form_helper.rb
	app/models/issue_tracker.rb
	app/models/notification_services/gtalk_service.rb
	app/models/notification_services/pushover_service.rb
	app/models/problem.rb
	config/routes.rb
	lib/hoptoad.rb
@stevecrozz
Copy link
Member

@stevecrozz stevecrozz commented Oct 30, 2015

@funglaub I would like to pull this in because I'm clearly not getting around to moving these issue trackers into external services. Can you rebase this PR into just one commit?

@funglaub funglaub closed this Nov 3, 2015
@funglaub funglaub deleted the timeout-xmpp4r branch Nov 3, 2015
@funglaub
Copy link
Contributor Author

@funglaub funglaub commented Nov 3, 2015

@stevecrozz I screwed it up, sorry. Please see #989.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants