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

gh-88427: Deprecate socket.SocketType #126272

Draft
wants to merge 48 commits into
base: main
Choose a base branch
from
Draft

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Nov 1, 2024

I propose to deprecate socket.SocketType. There's no reason to publicly expose _socket.socket separately from socket.socket, the only type that moat users should care about.
SocketType isn't needed for type checking; socket.socket is itself a class and can be used in type annotations.

cc @JelleZijlstra

@rruuaanng rruuaanng changed the title gh88427: Deprecated socket.SocketType gh-88427: Deprecated socket.SocketType Nov 1, 2024
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SocketType isn't deprecated right now, so we can't remove it yet.

Please update the PR to raise a DeprecationWarning when accessing a SocketType (you can do this via a PEP-562 __getattr__), and then add the documentation notes.

@rruuaanng
Copy link
Contributor Author

Maybe we can wait for the promoter to answer. I won't open this PR before I decide whether to abandon or remove it.

@JelleZijlstra
Copy link
Member

I looked at the code again and agree with my opinion from 2021: it should be deprecated and eventually removed.

It could be useful to do a code search (e.g., GitHub code search, Hugo's top 5k pypi packages tool) to see how the name is being used.

A possible approach to deprecation could be to remove the current line adding it to _socket, and add a module __getattr__ to both the _socket and socket modules that raises a DeprecationWarning and then returns _socket.socket.

@rruuaanng
Copy link
Contributor Author

Okay, I'll re-revise this PR so that it throws a warning when calling this type. Call me back in this PR when we consider removing it in the future :)

@rruuaanng
Copy link
Contributor Author

It seems that the module __getattr__ only works for attributes that do not exist, which makes it necessary to remove the export in socketmodule. But it triggers a deprecation warning. Emmm...

@picnixz
Copy link
Member

picnixz commented Nov 2, 2024

It seems that the module getattr only works for attributes that do not exist, which makes it necessary to remove the export in socketmodule. But it triggers a deprecation warning. Emmm...

It works for any kind of attributes. Please, read PEP-562 for examples on how to use it.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 2, 2024

It seems that the module getattr only works for attributes that do not exist, which makes it necessary to remove the export in socketmodule. But it triggers a deprecation warning. Emmm...

It works for any kind of attributes. Please, read PEP-562 for examples on how to use it.

I read the document and modified it. But why does it trigger this

Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\pythoninfo.py", line 1062, in collect_info
    collect_func(info_add)
    ~~~~~~~~~~~~^^^^^^^^^^
  File "D:\a\cpython\cpython\Lib\test\pythoninfo.py", line 727, in collect_test_socket
    from test import test_socket
  File "D:\a\cpython\cpython\Lib\test\test_socket.py", line 201, in <module>
    HAVE_SOCKET_CAN = _have_socket_can()
  File "D:\a\cpython\cpython\Lib\test\test_socket.py", line 104, in _have_socket_can
    s = socket.socket(socket.PF_CAN, socket.SOCK_RAW, socket.CAN_RAW)
  File "D:\a\cpython\cpython\Lib\socket.py", line 242, in __init__
    _socket.socket.__init__(self, family, type, proto, fileno)
    ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'NoneType' object cannot be interpreted as an integer

Edit
It seems to be a consistent bug since I opened the PR.

@ZeroIntensity
Copy link
Member

I think you need to raise an AttributeError from the __getattr__ if it isn't SocketType.

@rruuaanng rruuaanng marked this pull request as ready for review November 2, 2024 01:50
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rruuaanng: Did you try your change before updating the PR? It doesn't work, no warning is emitted:

$ ./python
>>> import _socket
>>> _socket.SocketType
<class '_socket.socket'>

I suggest to mark the PR as a draft since it changes many times in a short time.

@rruuaanng
Copy link
Contributor Author

@vstinner
I forgot the most important step, removing its definition, and now it can successfully trigger deprecation ;-)

>>> import _socket
>>> _socket.SocketType
<python-input-1>:1: DeprecationWarning: It will be removed in 3.16. Use socket.socket type instead
  _socket.SocketType
<class '_socket.socket'>

@rruuaanng rruuaanng marked this pull request as draft November 19, 2024 13:53
@vstinner
Copy link
Member

I forgot the most important step

Right. Please test your change before publishing it.

@rruuaanng rruuaanng marked this pull request as ready for review December 8, 2024 12:37
@rruuaanng
Copy link
Contributor Author

Please allow me to check again to see if there are any unresolved comments to decide whether we should remove the DO_NOT_MERGE tag.

@vstinner
Copy link
Member

vstinner commented Dec 9, 2024

test_socket and test_pickle are failing on the CI Ubuntu.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Dec 10, 2024

Oh.... I don't understand, I was looking for what was throwing the error so I ran the failing tests locally and it seemed like they all worked fine on my computer.

test_socket.py

PS E:\code\github\cpython\alpha\cpython-main> ./python -m unittest Lib\test\test_socket.py
Running Release|x64 interpreter...
.ss.sssssssss.......sssssssss.......................sss...ssss........ssssss.....................s...........................E:\code\github\cpython\alpha\cpython-main\Lib\socket.py:74: DeprecationWarning: _socket.SocketType is deprecated and will be removed in Python 3.16. Use socket.socket instead
  return _socket.SocketType
.s........s...s............sss.......s..........ssssss..s.s.ssssssssssss..........sssssss...............s..ss.....sssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss...........ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss...........s..ss..ssssss.s.....sssssssss...............................................
----------------------------------------------------------------------
Ran 732 tests in 69.918s

OK (skipped=485)

test_pickle.py

PS E:\code\github\cpython\alpha\cpython-main> ./python -m unittest Lib\test\test_pickle.py
Running Release|x64 interpreter...
...........s.................................................................................................................................s...........................................................................................................................................................s........................sss..............sss.......Can't import module 'multiprocessing.popen_fork': module 'os' has no attribute 'WNOHANG'
No module named '_gdbm'
No module named '_gdbm'
No module named 'dbm.bsd'
No module named '_dummy_thread'
No module named '_dbm'
No module named '_dbm'
.No module named '_dummy_thread'
No module named 'dbm.bsd'
No module named '_dbm'
No module named '_gdbm'
No module named '_dbm'
No module named '_gdbm'
..Can't import module 'multiprocessing.popen_fork': module 'os' has no attribute 'WNOHANG'
E:\code\github\cpython\alpha\cpython-main\Lib\socket.py:74: DeprecationWarning: _socket.SocketType is deprecated and will be removed in Python 3.16. Use socket.socket instead
  return _socket.SocketType
.No module named '_dummy_thread'
No module named '_dummy_thread'
No module named 'dbm.bsd'
No module named 'dbm.bsd'
No module named '_dbm'
No module named '_dbm'
No module named '_gdbm'
No module named '_gdbm'
.Can't import module 'multiprocessing.popen_fork': module 'os' has no attribute 'WNOHANG'
E:\code\github\cpython\alpha\cpython-main\Lib\socket.py:74: DeprecationWarning: _socket.SocketType is deprecated and will be removed in Python 3.16. Use socket.socket instead
  return _socket.SocketType
....s..........................................................................................................................s...............................sssss....sss......................sss...........................................................................................................s............................................................................................................................s......................................................sss..............sss........
----------------------------------------------------------------------
Ran 865 tests in 13.836s

OK (skipped=30)

I tried removing my changes and ran test_pickle.py again, but the only difference was that the deprecation was gone.

PS E:\code\github\cpython\alpha\cpython-main> ./python -m unittest Lib\test\test_pickle.py
Running Release|x64 interpreter...
...........s.................................................................................................................................s...........................................................................................................................................................s........................sss..............sss.......No module named '_gdbm'
Can't import module 'multiprocessing.popen_fork': module 'os' has no attribute 'WNOHANG'
No module named '_dbm'
No module named 'dbm.bsd'
No module named '_dummy_thread'
No module named '_gdbm'
No module named '_dbm'
.No module named '_dummy_thread'
No module named 'dbm.bsd'
No module named '_dbm'
No module named '_gdbm'
No module named '_dbm'
No module named '_gdbm'
..Can't import module 'multiprocessing.popen_fork': module 'os' has no attribute 'WNOHANG'
.No module named '_dummy_thread'
No module named '_dummy_thread'
No module named 'dbm.bsd'
No module named 'dbm.bsd'
No module named '_dbm'
No module named '_dbm'
No module named '_gdbm'
No module named '_gdbm'
.Can't import module 'multiprocessing.popen_fork': module 'os' has no attribute 'WNOHANG'
....s..........................................................................................................................s...............................sssss....sss......................sss...........................................................................................................s............................................................................................................................s......................................................sss..............sss........
----------------------------------------------------------------------
Ran 865 tests in 15.947s

OK (skipped=30)

test_socket.py passes the test perfectly fine and throws the deprecation, but test_pickle.py seems to throw an error that seems unrelated to the PR. But it is undeniable that my CI did not pass :(

Edit

test_pickle.py threw the same exception as my local test results on all three platforms.

PyErr_Warn(PyExc_DeprecationWarning, "_socket.SocketType is deprecated and "
"will be removed in Python 3.16. "
"Use socket.socket instead");
return state != NULL ? (PyObject *)state->sock_type : NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should sock_type get INCREF'd? I'm assuming its a static type and therefore immortal, but we should add a comment clarifying that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to search for situations where sock_type is used in socketmodule.c. In the scenario where Py_DECREF is used, only socket_clear uses Py_DECREF. And sock_type does not use Py_INCREF. And in other parts, it seems that there is no special comment.

@rruuaanng rruuaanng marked this pull request as draft December 12, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants