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

No way to disable socket timeouts in httplib, etc. #46703

Closed
jjlee mannequin opened this issue Mar 21, 2008 · 15 comments
Closed

No way to disable socket timeouts in httplib, etc. #46703

jjlee mannequin opened this issue Mar 21, 2008 · 15 comments
Assignees
Labels
stdlib Python modules in the Lib dir

Comments

@jjlee
Copy link
Mannequin

jjlee mannequin commented Mar 21, 2008

BPO 2451
Nosy @facundobatista, @gpshead, @benjaminp
Files
  • issue2451.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/facundobatista'
    closed_at = <Date 2008-06-07.13:37:00.067>
    created_at = <Date 2008-03-21.23:16:17.525>
    labels = ['library']
    title = 'No way to disable socket timeouts in httplib, etc.'
    updated_at = <Date 2008-06-07.13:37:00.043>
    user = 'https://bugs.python.org/jjlee'

    bugs.python.org fields:

    activity = <Date 2008-06-07.13:37:00.043>
    actor = 'facundobatista'
    assignee = 'facundobatista'
    closed = True
    closed_date = <Date 2008-06-07.13:37:00.067>
    closer = 'facundobatista'
    components = ['Library (Lib)']
    creation = <Date 2008-03-21.23:16:17.525>
    creator = 'jjlee'
    dependencies = []
    files = ['9902']
    hgrepos = []
    issue_num = 2451
    keywords = ['patch']
    message_count = 15.0
    messages = ['64293', '64297', '64300', '64301', '64525', '64585', '64604', '64750', '64751', '64752', '64756', '65805', '65808', '67502', '67806']
    nosy_count = 5.0
    nosy_names = ['facundobatista', 'gregory.p.smith', 'jjlee', 'amak', 'benjamin.peterson']
    pr_nums = []
    priority = 'high'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue2451'
    versions = ['Python 2.6']

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Mar 21, 2008

    The new timeout support in 2.6 makes use of new function
    socket.create_connection(). socket.create_connection() provides no way
    to disable timeouts, other than by relying on socket.getdefaulttimeout()
    returning None. This is unfortunate, because it was the purpose of the
    new timeout support to allow control of timeouts without reliance on
    global state.

    setdefaultsocket.create_connection() should always call
    sock.settimeout() with the timeout passed to create_connection(), unless
    a special non-None value is passed indicating that the global default is
    to be used. Specific modules may then use that special non-None value
    where required, to preserve backwards-compatibility.

    @jjlee jjlee mannequin added the stdlib Python modules in the Lib dir label Mar 21, 2008
    @facundobatista
    Copy link
    Member

    When the semantics of timout in htmllib (and the other libraries) were
    discussed, the community approved the following behaviour:

    • setdefaulttimeout() was a hack to allow to set the timeout when
      nothing else is available.

    • Now that you can pass the timeout when creating the connection, you
      shouldn't use the default setting.

    So, as to enhance practicality and ease to use, when you pass a timeout
    value it will set up the timeout in the socket. When you don't pass
    anything, or pass None, it will not set anything. And if you set
    previously a default value, you can not override it through this
    parameter: you shouldn't been setting the default in first place.

    Regards,

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Mar 22, 2008

    I see this thread:

    http://www.gossamer-threads.com/lists/python/dev/552292

    But I don't see an explanation of this API decision there that I understand.

    *Because* socket.setdefaulttimeout() is a hack for when nothing else is
    available, there should be a way to avoid that global state.

    You say that "Now that you can pass the timeout when creating the
    connection, you shouldn't use the default setting.". That's true, for
    new code, and for code is able to immediately change -- indeed, that
    always has been true. Code exists that makes use of
    socket.setdefaulttimeout(), or requires use of it in order to set a
    timeout. Can you explain why this state of affairs makes it necessary
    to force this global state on users of httplib?

    @facundobatista
    Copy link
    Member

    Two or three threads run in parallel at that time for this issue, don't
    remember exactly where this was decided.

    *Because* socket.setdefaulttimeout() is a hack for when nothing else is
    available, there should be a way to avoid that global state.

    Yes: don't call setdefaulttimeout().

    socket.setdefaulttimeout(), or requires use of it in order to set a
    timeout. Can you explain why this state of affairs makes it necessary
    to force this global state on users of httplib?

    The issue is that to allow caller to override the default state you
    should pass it None, but None is for default timeout value in the call,
    so you should be passing a specific object to mean "override default
    timeout", etc... all this is well explained in that thread.

    Lot of suggestions were handled, and the final decision was that all
    that behaviours will complicate unnecessarily the semantics here (with
    the cost of not being able to override the global default).

    I suggest you to raise the discussion again in python-dev if you want
    this decided behaviour to change.

    @amak
    Copy link
    Mannequin

    amak mannequin commented Mar 25, 2008

    The best solution to this problem was pointed out by Josiah Carlson at
    the time; Facundo expressed a preference for Josiah's solution, so it
    must have been an oversight that the solution didn't make it into the patch.

    http://www.gossamer-threads.com/lists/python/dev/555108?do=post_view_threaded#555108
    http://www.gossamer-threads.com/lists/python/dev/555110?do=post_view_threaded#555110

    Repeating the proposed solution here

    sentinel = object()
    
    def create_connection(address, timeout=sentinel):
    ...
        if timeout is not sentinel:
            sock.settimeout(timeout)
    ... 

    Perhaps Fecundo could be persuaded to implement this solution?

    Alan.

    @facundobatista
    Copy link
    Member

    Mmm.... it seems that not only overlooked this final agreement, but also
    forgot it! Bloody brain, :(

    I'll happily review any proposed patch for this. Alan, maybe you can be
    persuaded to submit one? <.5 wink>

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Mar 27, 2008

    Great. I'll try to submit a patch this weekend.

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Mar 30, 2008

    I've attached a patch.

    My patch introduces one minor issue: it's an inconvenience when wrapping
    objects if special default values like socket._GLOBAL_DEFAULT_TIMEOUT
    are not public. However, I think it's not worth making that special
    value public in this case, because it's not needed by code that does not
    support the socket.getdefaulttimeout() global default timeout.

    Patch description
    -----------------

    • Change the timeout arguments that were introduced with Facundo's 2.6
      timeout changes so that they have the same meaning as the argument of
      socket.socket.settimeout() . Specifically, None means "no timeout"
      (previously, there was no way to request that), and there is no special
      public timeout value meaning "use the global default
      socket.getdefaulttimeout() value" (previously, you could pass None to
      request that). This affects socket, urllib2, urllib (only
      urllib.ftpwrapper, for urllib2's benefit, urllib public interface and
      behaviour is unchanged), httplib, ftplib, telnetlib, poplib, and smtplib.

    • Fix a test bug: test_urllib2net changed global state by calling
      urllib2.install_opener(), which interfered with other tests.

    • In tests, where possible, close sockets by calling high-level methods
      (e.g. call ftplib.FTP.close(), rather than poking into the object's
      internals to .close() the socket directly).

    • Inconsistent defaulting behaviour in telnetlib was introduced with
      the timeout changes (r54608). Made timeout behave like port in terms of
      defaulting behaviour.

    • Improve socket.create_connection() documentation. Some of these
      changes need to be propagated to the individual protocol modules that
      call this function (e.g. httplib).

    • Document return value

    • Be more PEP-8-compliant ("Connect to...", not "Connects to...")

    • Remove this sentence, since it seems out of place in API
      documentation and unlikely to be true: "Especially useful for
      higher-level protocols, it is not normally used directly from
      application-level code."

    • Reword to remove any doubt that the timeout applies to (almost) all
      blocking operations, not just .connect()

    • Rephrase timeout parameter description for better English style.

    • Note that create_connection() is a convenience function (rather than
      part of the thin wrapper around the C API).

    • Make the docstring for create_connection() match the official reST
      API docs.

    Unresolved issues with the new 2.6 timeout functionality
    --------------------------------------------------------

    • http://bugs.python.org/issue2452

    • I did not propagate the changes to socket.create_connection() docs to
      all the protocol modules (httplib, etc.). Probably this change should
      be committed separately -- I ran out of time.

    • References in the docs to "the global default timeout setting" are
      vague. They should specifically refer to socket.getdefaulttimeout() .
      This should be done in such a way as to also fix the lack of
      documentation of the None special value in the protocol modules
      documentation (httplib, etc.). I should have fixed that as part of this
      patch, but ran out of time -- sorry!

    • Inconsistency: CacheFTPHandler has per-handler timeout, per-request
      timeout is ignored! Doc/library/urllib2.rst says (in two places):

    """This actually only work for HTTP, HTTPS, FTP and FTPS connections."""

    That's not true. (What about CacheFTPHandler, for instance?) It's also
    unclear why it refers to *connections* rather than URL schemes, handler
    classes, and the network operations they perform. (there's also a
    little grammatical error here -- s/work/works/)

    • API weirdness: Only some, special, urllib2.Request objects may be
      passed to handlers, because the constructor does not create objects with
      a .timeout. Should it really be per-request anyway (I'm not sure)?

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Mar 30, 2008

    Should I also have selected "Python 3.0" from the "Versions" list, BTW?
    Don't know what the proper process is ATM...

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Mar 30, 2008

    Me:

    """
    This should be done in such a way as to also fix the lack of
    documentation of the None special value in the protocol modules
    documentation (httplib, etc.). I should have fixed that as part of this
    patch, but ran out of time -- sorry!
    """

    Erm, possibly the meaning and allowed values of the timeout arguments
    would be more naturally documented by pointing to the socket
    .settimeout() method docs, actually.

    @benjaminp
    Copy link
    Contributor

    Marking just 2.6 is fine. The fix will be merged into 3.0

    @jjlee
    Copy link
    Mannequin Author

    jjlee mannequin commented Apr 25, 2008

    Facundo, are you going to review this?

    @facundobatista
    Copy link
    Member

    Yes! :)

    @facundobatista facundobatista self-assigned this Apr 25, 2008
    @facundobatista
    Copy link
    Member

    Commited (part of this) patch on r63788.

    A lot of small details weren't commited, in a big change like this, the
    best is to minimize the changes.

    What I have left from this commit, but plan to do it later is a fix to
    test_urllib2net.py (this is why I'm not closing this issue).

    Thank you!!

    @facundobatista
    Copy link
    Member

    Applied the rest of the patch regarding test_urllib2net.py.

    Thank you!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants