pyepics / newportxps

python support code for NewportXPS drivers
BSD 2-Clause "Simplified" License
20 stars 17 forks source link

remove default socket timeout #15

Closed mikessut closed 4 months ago

mikessut commented 1 year ago

Is this default socket timeout really necessary? It was causing my program to terminate early. I've removed it here and it seems to still work.

If it is necessary, is there a better way to set it for the sockets used instead of setting it for all sockets used by python?

newville commented 1 year ago

@MikeSuttonWork Oh, I guess I did not realize that this would set a global value, but I see now that it does.
I might suggest:

        try:
            s_timeout = socket.getdefaulttimeout()
            socket.setdefaulttimeout(5)
            host = socket.gethostbyname(host)
        except:
            raise ValueError('Could not resolve XPS name %s' % host)
        finally:
            socket.setdefaulttimeout(s_timeout)

That is, we don't want gethostbyname() to fail or hang indefinitely. But we should clean up and not let a global state value be set.

mikessut commented 1 year ago

For me, even with no timeout set, it looks like socket.gethostbyname() doesn't hang indefinitely if it can't resolve the hostname; rather it raises a socket.gaierror. Seems like that would be an acceptable solution.

Maybe that behavior is platform dependent though -- I'm testing on Windows.

On Thu, Oct 27, 2022 at 6:20 PM Matt Newville @.***> wrote:

@MikeSuttonWork https://github.com/MikeSuttonWork Oh, I guess I did not realize that this would set a global value, but I see now that it does. I might suggest:

    try:
        s_timeout = socket.getdefaulttimeout()
        socket.setdefaulttimeout(5)
        host = socket.gethostbyname(host)
    except:
        raise ValueError('Could not resolve XPS name %s' % host)
    finally:
        socket.setdefaulttimeout(s_timeout)

That is, we don't want gethostbyname() to fail or hang indefinitely. But we should clean up and not let a global state value be set.

— Reply to this email directly, view it on GitHub https://github.com/pyepics/newportxps/pull/15#issuecomment-1294258713, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARFNBGA6TLFLQVNUD5HZX3WFML6XANCNFSM6AAAAAARQQWGEI . You are receiving this because you authored the thread.Message ID: @.***>

newville commented 1 year ago

@mikessut Yeah, when I look at that code it looks pretty old to me, and so certainly from Python 2.6 or 2.7 days. I'm not at all certain that setting the default timeout is necessary with Python 3.7 (or 3.8) or higher. Like, I am not sure that we really need the hostname if we have the IP address. That seems convenient but maybe not necessary....

I guess I'm slightly reluctant to remove it, but maybe just "ripping the bandaid off and letting it bleed" is ok here.

newville commented 4 months ago

this is now solved.