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

Add timeout to urlopen calls #136

Merged
merged 4 commits into from
Apr 4, 2014
Merged

Add timeout to urlopen calls #136

merged 4 commits into from
Apr 4, 2014

Conversation

eykamp
Copy link
Contributor

@eykamp eykamp commented Apr 3, 2014

Occasionally, a WxS server will be unresponsive, and can cause urlopen to hang. For example, wms.jpl.nasa.gov/wms.cgi?service=WFS&request=GetCapabilities&version=1.0.0 never returns (at least not this week!) The urlopen function can take a timeout parameter, so in almost every place where this is called, I added a timeout, which can be passed into the calling function, and defaults to 30 seconds.

The one place I did not make this change was in wfs200.py :: getpropertyvalue() because I could see no good way to pass a timeout to that function without breaking the signature for existing functions.

I am not positive this is the best solution, but it totally fixed the hanging issues I was experiencing.

@tomkralidis
Copy link
Member

1./ @eykamp: Can you rebase this off latest master? 7d26688 seems to be included in the PR (rewriting history)
2./ @kwilcox any comments here? Not sure if/how this goes with your work in #111

@kwilcox
Copy link
Member

kwilcox commented Apr 4, 2014

I do like the timeout idea, +1 for this.

I'll merge my changes in #111 when they are done, no worries there.

tomkralidis added a commit that referenced this pull request Apr 4, 2014
Add timeout to urlopen calls
@tomkralidis tomkralidis merged commit 465e42f into geopython:master Apr 4, 2014
@eykamp
Copy link
Contributor Author

eykamp commented Apr 4, 2014

Hi Tom,

I did the rebase, I think, but it also looks like you already accepted
the pull request, so... is there anything you want me to do with this?

Thanks,

Chris

On 04/03/2014 05:25 PM, Tom Kralidis wrote:

1./ @eykamp https://github.com/eykamp: Can you rebase this off
latest master? 7d26688
7d26688
seems to be included in the PR (rewriting history)
2./ @kwilcox https://github.com/kwilcox any comments here? Not sure
if/how this goes with your work in #111
#111


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

@tomkralidis
Copy link
Member

Yes I merged it thx.

eykamp pushed a commit to eykamp/OWSLib that referenced this pull request Aug 12, 2014
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.

None yet

5 participants