Skip to content

Commit a58b50f

Browse files
icingbagder
icing
authored and
bagder
committed
transfer: Curl_sendrecv() and event related improvements
- Renames Curl_readwrite() to Curl_sendrecv() to reflect that it is mainly about talking to the server, not reads or writes to the client. Add a `nowp` parameter since the single caller already has this. - Curl_sendrecv() now runs all possible operations whenever it is called and either it had been polling sockets or the 'select_bits' are set. POLL_IN/POLL_OUT are not always directly related to send/recv operations. Filters like HTTP/2, QUIC or TLS may monitor reverse directions. If a transfer does not want to send (KEEP_SEND), it will not do so, as before. Same for receives. - Curl_update_timer() now checks the absolute timestamp of an expiry and the last/new timeout to determine if the application needs to stop/start/restart its timer. This fixes edge cases where updates did not happen as they should have. - improved --test-event curl_easy_perform() simulation to handle situations where no sockets are registered but a timeout is in place. - fixed bug in events_socket() that complained about removing a socket that was unknown, when indeed it had removed the socket just before, only it was the last in the list - fixed conncache's internal handle to carry the multi instance (where the cache has one) so that operations on the closure handle trigger event callbacks correctly. - fixed conncache to not POLL_REMOVE a socket twice when a conneciton was closed. Closes #14561
1 parent 432f2fd commit a58b50f

File tree

12 files changed

+318
-283
lines changed

12 files changed

+318
-283
lines changed

‎lib/cf-socket.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,9 @@ CURLcode Curl_socket_open(struct Curl_easy *data,
404404
static int socket_close(struct Curl_easy *data, struct connectdata *conn,
405405
int use_callback, curl_socket_t sock)
406406
{
407+
if(CURL_SOCKET_BAD == sock)
408+
return 0;
409+
407410
if(use_callback && conn && conn->fclosesocket) {
408411
int rc;
409412
Curl_multi_closed(data, sock);

‎lib/conncache.c‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,12 @@ int Curl_conncache_init(struct conncache *connc,
131131
if(!connc->closure_handle)
132132
return 1; /* bad */
133133
connc->closure_handle->state.internal = true;
134+
/* TODO: this is quirky. We need an internal handle for certain
135+
* operations, but we do not add it to the multi (if there is one).
136+
* But we give it the multi so that socket event operations can work.
137+
* Probably better to have an internal handle owned by the multi that
138+
* can be used for conncache operations. */
139+
connc->closure_handle->multi = multi;
134140
#ifdef DEBUGBUILD
135141
if(getenv("CURL_DEBUG"))
136142
connc->closure_handle->set.verbose = true;
@@ -146,6 +152,10 @@ int Curl_conncache_init(struct conncache *connc,
146152
void Curl_conncache_destroy(struct conncache *connc)
147153
{
148154
if(connc) {
155+
if(connc->closure_handle) {
156+
connc->closure_handle->multi = NULL;
157+
Curl_close(&connc->closure_handle);
158+
}
149159
Curl_hash_destroy(&connc->hash);
150160
connc->multi = NULL;
151161
}
@@ -611,7 +621,8 @@ static void connc_close_all(struct conncache *connc)
611621

612622
sigpipe_apply(data, &pipe_st);
613623
Curl_hostcache_clean(data, data->dns.hostcache);
614-
Curl_close(&data);
624+
connc->closure_handle->multi = NULL;
625+
Curl_close(&connc->closure_handle);
615626
sigpipe_restore(&pipe_st);
616627
}
617628

@@ -980,14 +991,6 @@ static void connc_disconnect(struct Curl_easy *data,
980991

981992
Curl_attach_connection(data, conn);
982993

983-
if(connc && connc->multi && connc->multi->socket_cb) {
984-
struct easy_pollset ps;
985-
/* With an empty pollset, all previously polled sockets will be removed
986-
* via the multi_socket API callback. */
987-
memset(&ps, 0, sizeof(ps));
988-
(void)Curl_multi_pollset_ev(connc->multi, data, &ps, &conn->shutdown_poll);
989-
}
990-
991994
connc_run_conn_shutdown_handler(data, conn);
992995
if(do_shutdown) {
993996
/* Make a last attempt to shutdown handlers and filters, if

‎lib/easy.c‎

Lines changed: 46 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -391,25 +391,22 @@ struct events {
391391
int running_handles; /* store the returned number */
392392
};
393393

394+
#define DEBUG_EV_POLL 0
395+
394396
/* events_timer
395397
*
396398
* Callback that gets called with a new value when the timeout should be
397399
* updated.
398400
*/
399-
400401
static int events_timer(struct Curl_multi *multi, /* multi handle */
401402
long timeout_ms, /* see above */
402403
void *userp) /* private callback pointer */
403404
{
404405
struct events *ev = userp;
405406
(void)multi;
406-
if(timeout_ms == -1)
407-
/* timeout removed */
408-
timeout_ms = 0;
409-
else if(timeout_ms == 0)
410-
/* timeout is already reached! */
411-
timeout_ms = 1; /* trigger asap */
412-
407+
#if DEBUG_EV_POLL
408+
fprintf(stderr, "events_timer: set timeout %ldms\n", timeout_ms);
409+
#endif
413410
ev->ms = timeout_ms;
414411
ev->msbump = TRUE;
415412
return 0;
@@ -463,6 +460,7 @@ static int events_socket(struct Curl_easy *easy, /* easy handle */
463460
struct events *ev = userp;
464461
struct socketmonitor *m;
465462
struct socketmonitor *prev = NULL;
463+
bool found = FALSE;
466464

467465
#if defined(CURL_DISABLE_VERBOSE_STRINGS)
468466
(void) easy;
@@ -472,7 +470,7 @@ static int events_socket(struct Curl_easy *easy, /* easy handle */
472470
m = ev->list;
473471
while(m) {
474472
if(m->socket.fd == s) {
475-
473+
found = TRUE;
476474
if(what == CURL_POLL_REMOVE) {
477475
struct socketmonitor *nxt = m->next;
478476
/* remove this node from the list of monitored sockets */
@@ -481,7 +479,6 @@ static int events_socket(struct Curl_easy *easy, /* easy handle */
481479
else
482480
ev->list = nxt;
483481
free(m);
484-
m = nxt;
485482
infof(easy, "socket cb: socket %" CURL_FORMAT_SOCKET_T
486483
" REMOVED", s);
487484
}
@@ -499,12 +496,13 @@ static int events_socket(struct Curl_easy *easy, /* easy handle */
499496
prev = m;
500497
m = m->next; /* move to next node */
501498
}
502-
if(!m) {
499+
500+
if(!found) {
503501
if(what == CURL_POLL_REMOVE) {
504-
/* this happens a bit too often, libcurl fix perhaps? */
505-
/* fprintf(stderr,
506-
"%s: socket %d asked to be REMOVED but not present!\n",
507-
__func__, s); */
502+
/* should not happen if our logic is correct, but is no drama. */
503+
DEBUGF(infof(easy, "socket cb: asked to REMOVE socket %"
504+
CURL_FORMAT_SOCKET_T "but not present!", s));
505+
DEBUGASSERT(0);
508506
}
509507
else {
510508
m = malloc(sizeof(struct socketmonitor));
@@ -565,27 +563,43 @@ static CURLcode wait_or_timeout(struct Curl_multi *multi, struct events *ev)
565563
int pollrc;
566564
int i;
567565
struct curltime before;
568-
struct curltime after;
569566

570567
/* populate the fds[] array */
571568
for(m = ev->list, f = &fds[0]; m; m = m->next) {
572569
f->fd = m->socket.fd;
573570
f->events = m->socket.events;
574571
f->revents = 0;
575-
/* fprintf(stderr, "poll() %d check socket %d\n", numfds, f->fd); */
572+
#if DEBUG_EV_POLL
573+
fprintf(stderr, "poll() %d check socket %d\n", numfds, f->fd);
574+
#endif
576575
f++;
577576
numfds++;
578577
}
579578

580579
/* get the time stamp to use to figure out how long poll takes */
581580
before = Curl_now();
582581

583-
/* wait for activity or timeout */
584-
pollrc = Curl_poll(fds, (unsigned int)numfds, ev->ms);
585-
if(pollrc < 0)
586-
return CURLE_UNRECOVERABLE_POLL;
587-
588-
after = Curl_now();
582+
if(numfds) {
583+
/* wait for activity or timeout */
584+
#if DEBUG_EV_POLL
585+
fprintf(stderr, "poll(numfds=%d, timeout=%ldms)\n", numfds, ev->ms);
586+
#endif
587+
pollrc = Curl_poll(fds, (unsigned int)numfds, ev->ms);
588+
#if DEBUG_EV_POLL
589+
fprintf(stderr, "poll(numfds=%d, timeout=%ldms) -> %d\n",
590+
numfds, ev->ms, pollrc);
591+
#endif
592+
if(pollrc < 0)
593+
return CURLE_UNRECOVERABLE_POLL;
594+
}
595+
else {
596+
#if DEBUG_EV_POLL
597+
fprintf(stderr, "poll, but no fds, wait timeout=%ldms\n", ev->ms);
598+
#endif
599+
pollrc = 0;
600+
if(ev->ms > 0)
601+
Curl_wait_ms(ev->ms);
602+
}
589603

590604
ev->msbump = FALSE; /* reset here */
591605

@@ -618,12 +632,17 @@ static CURLcode wait_or_timeout(struct Curl_multi *multi, struct events *ev)
618632
}
619633
}
620634

621-
if(!ev->msbump) {
635+
636+
if(!ev->msbump && ev->ms >= 0) {
622637
/* If nothing updated the timeout, we decrease it by the spent time.
623638
* If it was updated, it has the new timeout time stored already.
624639
*/
625-
timediff_t timediff = Curl_timediff(after, before);
640+
timediff_t timediff = Curl_timediff(Curl_now(), before);
626641
if(timediff > 0) {
642+
#if DEBUG_EV_POLL
643+
fprintf(stderr, "poll timeout %ldms not updated, decrease by "
644+
"time spent %ldms\n", ev->ms, (long)timediff);
645+
#endif
627646
if(timediff > ev->ms)
628647
ev->ms = 0;
629648
else
@@ -656,7 +675,7 @@ static CURLcode easy_events(struct Curl_multi *multi)
656675
{
657676
/* this struct is made static to allow it to be used after this function
658677
returns and curl_multi_remove_handle() is called */
659-
static struct events evs = {2, FALSE, 0, NULL, 0};
678+
static struct events evs = {-1, FALSE, 0, NULL, 0};
660679

661680
/* if running event-based, do some further multi inits */
662681
events_setup(multi, &evs);
@@ -1121,7 +1140,7 @@ CURLcode curl_easy_pause(struct Curl_easy *data, int action)
11211140
(data->mstate == MSTATE_PERFORMING ||
11221141
data->mstate == MSTATE_RATELIMITING));
11231142
/* Unpausing writes is detected on the next run in
1124-
* transfer.c:Curl_readwrite(). This is because this may result
1143+
* transfer.c:Curl_sendrecv(). This is because this may result
11251144
* in a transfer error if the application's callbacks fail */
11261145

11271146
/* Set the new keepon state, so it takes effect no matter what error

‎lib/http.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4441,7 +4441,8 @@ static CURLcode cr_exp100_read(struct Curl_easy *data,
44414441
}
44424442
/* We are now waiting for a reply from the server or
44434443
* a timeout on our side IFF the request has been fully sent. */
4444-
DEBUGF(infof(data, "cr_exp100_read, start AWAITING_CONTINUE"));
4444+
DEBUGF(infof(data, "cr_exp100_read, start AWAITING_CONTINUE, "
4445+
"timeout %ldms", data->set.expect_100_timeout));
44454446
ctx->state = EXP100_AWAITING_CONTINUE;
44464447
ctx->start = Curl_now();
44474448
Curl_expire(data, data->set.expect_100_timeout, EXPIRE_100_TIMEOUT);

0 commit comments

Comments
 (0)