Skip to content

Commit 931757f

Browse files
tannewttannewt
committed
Improve web workflow responsiveness
1. Run the socket select task at the same priority as CP. This is needed because it queues up the background work. Without it, CP needed to sleep to let the lower priority task go. 2. Close the active socket on disconnect. This prevents looping over a disconnected but not closed socket. Fixes adafruit#6610. Fixes adafruit#6613
1 parent bc926b0 commit 931757f

2 files changed

Lines changed: 30 additions & 24 deletions

File tree

‎ports/espressif/common-hal/socketpool/Socket.c‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -60,32 +60,35 @@ STATIC void socket_select_task(void *arg) {
6060
FD_SET(socket_change_fd, &errfds);
6161
int max_fd = socket_change_fd;
6262
for (size_t i = 0; i < MP_ARRAY_SIZE(open_socket_fds); i++) {
63-
if (open_socket_fds[i] < 0) {
63+
int sockfd = open_socket_fds[i];
64+
if (sockfd < 0) {
6465
continue;
6566
}
66-
max_fd = MAX(max_fd, open_socket_fds[i]);
67-
FD_SET(open_socket_fds[i], &readfds);
68-
FD_SET(open_socket_fds[i], &errfds);
67+
max_fd = MAX(max_fd, sockfd);
68+
FD_SET(sockfd, &readfds);
69+
FD_SET(sockfd, &errfds);
6970
}
7071

7172
int num_triggered = select(max_fd + 1, &readfds, NULL, &errfds, NULL);
72-
if (num_triggered < 0) {
73-
// Maybe bad file descriptor
74-
for (size_t i = 0; i < MP_ARRAY_SIZE(open_socket_fds); i++) {
75-
int sockfd = open_socket_fds[i];
76-
if (sockfd < 0) {
77-
continue;
78-
}
79-
if (FD_ISSET(sockfd, &errfds)) {
80-
int err;
81-
int optlen = sizeof(int);
82-
int ret = getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &err, (socklen_t *)&optlen);
83-
if (ret < 0) {
84-
open_socket_fds[i] = -1;
85-
// Try again.
86-
continue;
87-
}
88-
}
73+
// Check for bad file descriptor and queue up the background task before
74+
// circling around.
75+
if (num_triggered == -1 && errno == EBADF) {
76+
// One for the change fd and one for the closed socket.
77+
num_triggered = 2;
78+
}
79+
// Try and find the bad file and remove it from monitoring.
80+
for (size_t i = 0; i < MP_ARRAY_SIZE(open_socket_fds); i++) {
81+
int sockfd = open_socket_fds[i];
82+
if (sockfd < 0) {
83+
continue;
84+
}
85+
int err;
86+
int optlen = sizeof(int);
87+
int ret = getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &err, (socklen_t *)&optlen);
88+
if (ret < 0) {
89+
open_socket_fds[i] = -1;
90+
// Raise num_triggered so that we skip the assert and queue the background task.
91+
num_triggered = 2;
8992
}
9093
}
9194
assert(num_triggered >= 0);
@@ -117,13 +120,13 @@ void socket_user_reset(void) {
117120
user_socket[i] = false;
118121
}
119122
socket_change_fd = eventfd(0, 0);
120-
// This task runs at a lower priority than CircuitPython and is used to wake CircuitPython
121-
// up when any open sockets have data to read. It allows us to sleep otherwise.
123+
// Run this at the same priority as CP so that the web workflow background task can be
124+
// queued while CP is running. Both tasks can still sleep and, therefore, sleep overall.
122125
(void)xTaskCreateStaticPinnedToCore(socket_select_task,
123126
"socket_select",
124127
2 * configMINIMAL_STACK_SIZE,
125128
NULL,
126-
0, // Run this at IDLE priority. We only need it when CP isn't running (at 1).
129+
uxTaskPriorityGet(NULL),
127130
socket_select_stack,
128131
&socket_select_task_handle,
129132
xPortGetCoreID());

‎supervisor/shared/web_workflow/web_workflow.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,6 +1265,9 @@ void supervisor_web_workflow_background(void) {
12651265
// If we have a request in progress, continue working on it.
12661266
if (common_hal_socketpool_socket_get_connected(&active)) {
12671267
_process_request(&active, &active_request);
1268+
} else {
1269+
// Close the active socket if it is no longer connected.
1270+
common_hal_socketpool_socket_close(&active);
12681271
}
12691272
}
12701273

0 commit comments

Comments
 (0)