-
Notifications
You must be signed in to change notification settings - Fork 681
fix(socketcan): support socket file descriptors over 1023 #2057
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -815,9 +815,11 @@ def shutdown(self) -> None: | |
|
|
||
| def _recv_internal(self, timeout: float | None) -> tuple[Message | None, bool]: | ||
| try: | ||
| # get all sockets that are ready (can be a list with a single value | ||
| # being self.socket or an empty list if self.socket is not ready) | ||
| ready_receive_sockets, _, _ = select.select([self.socket], [], [], timeout) | ||
| # poll() avoids select.select()'s ValueError for fds >= FD_SETSIZE | ||
| poller = select.poll() | ||
| poller.register(self.socket, select.POLLIN) | ||
| timeout_ms = -1 if timeout is None else max(0, int(timeout * 1000)) | ||
| ready_receive_sockets = poller.poll(timeout_ms) | ||
| except OSError as error: | ||
| # something bad happened (e.g. the interface went down) | ||
| raise can.CanOperationError( | ||
|
|
@@ -857,9 +859,13 @@ def send(self, msg: Message, timeout: float | None = None) -> None: | |
| time_left = timeout | ||
| data = build_can_frame(msg) | ||
|
|
||
| # poll() avoids select.select()'s ValueError for fds >= FD_SETSIZE | ||
| poller = select.poll() | ||
| poller.register(self.socket, select.POLLOUT) | ||
|
|
||
| while time_left >= 0: | ||
| # Wait for write availability | ||
| ready = select.select([], [self.socket], [], time_left)[1] | ||
| ready = poller.poll(max(0, int(time_left * 1000))) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the |
||
| if not ready: | ||
| # Timeout | ||
| break | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ``SocketcanBus`` now uses ``select.poll()`` instead of ``select.select()`` so that socket file descriptors above ``FD_SETSIZE`` (1024 on glibc) no longer raise ``ValueError: filedescriptor out of range in select()``. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| #!/usr/bin/env python | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to create a new file to test this. Add a test to the existing socketcan tests. |
||
|
|
||
| """ | ||
| Regression tests for https://github.com/hardbyte/python-can/issues/2053. | ||
|
|
||
| ``SocketcanBus`` previously used ``select.select()``, which is limited by | ||
| glibc to file descriptors below ``FD_SETSIZE`` (1024) and raises | ||
| ``ValueError: filedescriptor out of range in select()`` for higher fds. | ||
|
|
||
| These tests verify that ``send()`` and ``recv()`` work with a socket whose | ||
| file descriptor exceeds 1023. | ||
| """ | ||
|
|
||
| import unittest | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import can | ||
| from can import Message | ||
| from can.interfaces.socketcan.socketcan import build_can_frame | ||
|
|
||
| from .config import IS_LINUX | ||
|
|
||
| HIGH_FD = 2048 | ||
|
|
||
|
|
||
| @unittest.skipUnless(IS_LINUX, "socketcan is only available on Linux") | ||
| class TestSocketcanHighFdLinux(unittest.TestCase): | ||
| """Verify SocketcanBus works when the underlying socket fd exceeds 1023.""" | ||
|
|
||
| def setUp(self): | ||
| patcher_create = patch("can.interfaces.socketcan.socketcan.create_socket") | ||
| patcher_bind = patch("can.interfaces.socketcan.socketcan.bind_socket") | ||
|
|
||
| self.mock_create_socket = patcher_create.start() | ||
| self.mock_bind_socket = patcher_bind.start() | ||
|
|
||
| self.mock_socket = MagicMock() | ||
| self.mock_socket.fileno.return_value = HIGH_FD | ||
| self.mock_create_socket.return_value = self.mock_socket | ||
|
|
||
| self.bus = can.Bus(interface="socketcan", channel="can0") | ||
|
|
||
| self.addCleanup(patcher_create.stop) | ||
| self.addCleanup(patcher_bind.stop) | ||
|
|
||
| def tearDown(self): | ||
| self.bus.shutdown() | ||
|
|
||
| @patch("can.interfaces.socketcan.socketcan.select.poll") | ||
| def test_send_high_fd(self, mock_poll_factory): | ||
| """send() succeeds when the socket fd > 1023.""" | ||
| poller = MagicMock() | ||
| # ``poll()`` returns a non-empty list to signal write readiness. | ||
| poller.poll.return_value = [(HIGH_FD, 4)] | ||
| mock_poll_factory.return_value = poller | ||
|
|
||
| msg = Message(arbitration_id=0x123, data=[1, 2, 3, 4, 5, 6, 7, 8]) | ||
| frame_data = build_can_frame(msg) | ||
| self.mock_socket.send.return_value = len(frame_data) | ||
|
|
||
| self.bus.send(msg) | ||
|
|
||
| self.mock_socket.send.assert_called_once_with(frame_data) | ||
| poller.register.assert_called_once() | ||
|
|
||
| @patch("can.interfaces.socketcan.socketcan.capture_message") | ||
| @patch("can.interfaces.socketcan.socketcan.select.poll") | ||
| def test_recv_high_fd(self, mock_poll_factory, mock_capture): | ||
| """recv() succeeds when the socket fd > 1023.""" | ||
| poller = MagicMock() | ||
| poller.poll.return_value = [(HIGH_FD, 1)] | ||
| mock_poll_factory.return_value = poller | ||
|
|
||
| expected_msg = Message( | ||
| arbitration_id=0x123, | ||
| data=[1, 2, 3, 4, 5, 6, 7, 8], | ||
| channel="can0", | ||
| timestamp=1000.0, | ||
| ) | ||
| mock_capture.return_value = expected_msg | ||
|
|
||
| msg = self.bus.recv(timeout=1.0) | ||
|
|
||
| self.assertIsNotNone(msg) | ||
| self.assertEqual(msg.arbitration_id, 0x123) | ||
| self.assertEqual(msg.data, bytearray([1, 2, 3, 4, 5, 6, 7, 8])) | ||
| mock_capture.assert_called_once_with(self.mock_socket, False) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You create new poll objects on every read and write. That can't be good for performance.
Create one poll object inside
__init__()and register both POLLIN and POLLOUT. Then here you call(fd, event) = self._poller.poll()and check whether the event containsPOLLIN.