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

Shouldn't throw exception when preview fetching timeouts #484

Merged
merged 2 commits into from
Mar 11, 2015

Conversation

oddlyfunctional
Copy link
Contributor

@lunks
How to test it:

  • Open a IRB or Pry console
  • Run the following code
require 'webrick'

server = WEBrick::HTTPServer.new :Port => 8000
server.mount_proc '/' do |req, res|
  sleep 9999
end
server.start

@lunks lunks had a problem deploying to dunnovc-staging-pr-484 March 11, 2015 00:17 Failure
@lunks
Copy link
Contributor

lunks commented Mar 11, 2015

IMHO it's taking too long. Can we reduce the time net persistent times out?

@lunks
Copy link
Contributor

lunks commented Mar 11, 2015

Also, there are at least one more error that can happen as well: http://bucket.dunnoapp.com/apps/54da0cd06432310009000000/problems/54fec4bf663133000f840000

@oddlyfunctional
Copy link
Contributor Author

That's funny, the Net::OpenTimeout error is addressed as the cause for the Net::HTTP::Persistent::Error. I'll try to catch it directly.

How many seconds do you think it's appropriate to wait before the timeout? 5?

@oddlyfunctional
Copy link
Contributor Author

Oops, actually the addressed cause is the Net::ReadTimeout.

@lunks
Copy link
Contributor

lunks commented Mar 11, 2015

There is an ongoing issue about this. It seems it is rescuing this timeout error and it shouldn't, but there is probably somewhere else raising this error as well. We should rescue it. I think 5s is more than enough.

@oddlyfunctional
Copy link
Contributor Author

There's no way to configure the read timeout for this gem, only the open timeout :/
Should I monkey patch the class or fork the project?

@lunks
Copy link
Contributor

lunks commented Mar 11, 2015

Fork it, add the option, and issue a PR.

@lunks lunks had a problem deploying to production March 11, 2015 19:50 Error
@oddlyfunctional
Copy link
Contributor Author

Done

@oddlyfunctional oddlyfunctional force-pushed the bugfix/fallback-thumbnail-when-timeout branch from 096029e to f65e4a8 Compare March 11, 2015 20:06
@lunks
Copy link
Contributor

lunks commented Mar 11, 2015

:shipit:

oddlyfunctional added a commit that referenced this pull request Mar 11, 2015
…imeout

Shouldn't throw exception when preview fetching timeouts
@oddlyfunctional oddlyfunctional merged commit e36cd6e into master Mar 11, 2015
@oddlyfunctional oddlyfunctional deleted the bugfix/fallback-thumbnail-when-timeout branch March 11, 2015 20:08
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