diff --git a/READMEs/README.coding.md b/READMEs/README.coding.md index 3dd68011..322b8e5b 100644 --- a/READMEs/README.coding.md +++ b/READMEs/README.coding.md @@ -44,7 +44,7 @@ clients). If you want to send something, do NOT just send it but request a callback when the socket is writeable using - - `lws_callback_on_writable(context, wsi)` for a specific `wsi`, or + - `lws_callback_on_writable(wsi)` for a specific `wsi`, or - `lws_callback_on_writable_all_protocol(protocol)` for all connections using that protocol to get a callback when next writeable. @@ -68,6 +68,25 @@ anything new for him. This is how unix shell piping works, you may have `cat a.txt | grep xyz > remote", but actually that does not cat anything from a.txt while remote cannot accept anything new. +@section oneper Only one lws_write per WRITEABLE callback + +From v2.5, lws strictly enforces only one lws_write() per WRITEABLE callback. + +You will receive a message about "Illegal back-to-back write of ... detected" +if there is a second lws_write() before returning to the event loop. + +This is because with http/2, the state of the network connection carrying a +wsi is unrelated to any state of the wsi. The situation on http/1 where a +new request implied a new tcp connection and new SSL buffer, so you could +assume some window for writes is no longer true. Any lws_write() can fail +and be buffered for completion by lws; it will be auto-completed by the +event loop. + +Note that if you are handling your own http responses, writing the headers +needs to be done with a separate lws_write() from writing any payload. That +means after writing the headers you must call `lws_callback_on_writable(wsi)` +and send any payload from the writable callback. + @section otherwr Do not rely on only your own WRITEABLE requests appearing Libwebsockets may generate additional `LWS_CALLBACK_CLIENT_WRITEABLE` events diff --git a/lib/handshake.c b/lib/handshake.c index e5a024b6..e1bafb60 100644 --- a/lib/handshake.c +++ b/lib/handshake.c @@ -91,6 +91,28 @@ lws_read(struct lws *wsi, unsigned char *buf, lws_filepos_t len) return 1; } + /* + * lws_h2_parser() may send something; when it gets the + * whole frame, it will want to perform some action + * involving a reply. But we may be in a partial send + * situation on the network wsi... + * + * Even though we may be in a partial send and unable to + * send anything new, we still have to parse the network + * wsi in order to gain tx credit to send, which is + * potentially necessary to clear the old partial send. + * + * ALL network wsi-specific frames are sent by PPS + * already, these are sent as a priority on the writable + * handler, and so respect partial sends. The only + * problem is when a stream wsi wants to send an, eg, + * reply headers frame in response to the parsing + * we will do now... the *stream wsi* must stall in a + * different state until it is able to do so from a + * priority on the WRITABLE callback, same way that + * file transfers operate. + */ + if (lws_h2_parser(wsi, buf, len, &body_chunk_len)) { lwsl_debug("%s: http2_parser bailed\n", __func__); goto bail; diff --git a/lib/http2/hpack.c b/lib/http2/hpack.c index f7fe9d36..825969e3 100644 --- a/lib/http2/hpack.c +++ b/lib/http2/hpack.c @@ -598,13 +598,13 @@ lws_hpack_dynamic_size(struct lws *wsi, int size) if (size > (int)nwsi->vhost->set.s[H2SET_HEADER_TABLE_SIZE]) { lwsl_notice("rejecting hpack dyn size %u\n", size); -#if defined(LWS_WITH_ESP32) +//#if defined(LWS_WITH_ESP32) size = nwsi->vhost->set.s[H2SET_HEADER_TABLE_SIZE]; -#else - lws_h2_goaway(nwsi, H2_ERR_COMPRESSION_ERROR, - "Asked for header table bigger than we told"); - goto bail; -#endif +//#else +// lws_h2_goaway(nwsi, H2_ERR_COMPRESSION_ERROR, +// "Asked for header table bigger than we told"); +// goto bail; +//#endif } dyn->virtual_payload_max = size; diff --git a/lib/http2/http2.c b/lib/http2/http2.c index cdbf5020..e875f501 100644 --- a/lib/http2/http2.c +++ b/lib/http2/http2.c @@ -1038,6 +1038,13 @@ update_end_headers: /* * The last byte of the whole frame has been handled. * Perform actions for frame completion. + * + * This is the crunch time for parsing that may have occured on a network + * wsi with a pending partial send... we may call lws_http_action() to send + * a response, conflicting with the partial. + * + * So in that case we change the wsi state and do the lws_http_action() in the + * WRITABLE handler as a priority. */ static int lws_h2_parse_end_of_frame(struct lws *wsi) @@ -1196,22 +1203,8 @@ lws_h2_parse_end_of_frame(struct lws *wsi) wsi->vhost->conn_stats.h2_trans++; - lwsl_info(" action start...\n"); - n = lws_http_action(h2n->swsi); - lwsl_info(" action result %d " - "(wsi->http.rx_content_remain %lld)\n", - n, h2n->swsi->http.rx_content_remain); - - /* - * Commonly we only managed to start a larger transfer that will - * complete asynchronously. In those cases we will hear about - * END_STREAM going out in the POLLOUT handler. - */ - if (n || h2n->swsi->h2.send_END_STREAM) { - lws_close_free_wsi(h2n->swsi, 0); - h2n->swsi = NULL; - break; - } + h2n->swsi->state = LWSS_HTTP2_DEFERRING_ACTION; + lws_callback_on_writable(h2n->swsi); break; case LWS_H2_FRAME_TYPE_DATA: @@ -1318,6 +1311,19 @@ lws_h2_parse_end_of_frame(struct lws *wsi) return 0; } +/* + * This may want to send something on the network wsi, which may be in the + * middle of a partial send. PPS sends are OK because they are queued to + * go through the WRITABLE handler already. + * + * The read parser for the network wsi has no choice but to parse its stream + * anyway, because otherwise it will not be able to get tx credit window + * messages. + * + * Therefore if we will send non-PPS, ie, lws_http_action() for a stream + * wsi, we must change its state and handle it as a priority in the + * POLLOUT handler instead of writing it here. + */ int lws_h2_parser(struct lws *wsi, unsigned char *in, lws_filepos_t inlen, lws_filepos_t *inused) diff --git a/lib/libwebsockets.c b/lib/libwebsockets.c index 87e29601..ab85b2ca 100644 --- a/lib/libwebsockets.c +++ b/lib/libwebsockets.c @@ -724,7 +724,7 @@ just_kill_connection: lws_sockfd_valid(wsi->desc.sockfd) && !LWS_LIBUV_ENABLED(context)) { lws_change_pollfd(wsi, LWS_POLLOUT, LWS_POLLIN); - wsi->state = LWSS_SHUTDOWN; + wsi->state = (wsi->state & ~0x1f) | LWSS_SHUTDOWN; lws_set_timeout(wsi, PENDING_TIMEOUT_SHUTDOWN_FLUSH, context->timeout_secs); diff --git a/lib/output.c b/lib/output.c index a0c781f8..525ba109 100644 --- a/lib/output.c +++ b/lib/output.c @@ -51,6 +51,23 @@ int lws_issue_raw(struct lws *wsi, unsigned char *buf, size_t len) unsigned int n; int m; + /* + * Detect if we got called twice without going through the + * event loop to handle pending. This would be caused by either + * back-to-back writes in one WRITABLE (illegal) or calling lws_write() + * from outside the WRITABLE callback (illegal). + */ + if (wsi->could_have_pending) { + lwsl_hexdump_level(LLL_ERR, buf, len); + lwsl_err("** %p: vh: %s, prot: %s, " + "Illegal back-to-back write of %lu detected...\n", + wsi, wsi->vhost->name, wsi->protocol->name, + (unsigned long)len); + // assert(0); + + return -1; + } + lws_stats_atomic_bump(wsi->context, pt, LWSSTATS_C_API_WRITE, 1); if (!len) @@ -62,13 +79,12 @@ int lws_issue_raw(struct lws *wsi, unsigned char *buf, size_t len) if (wsi->trunc_len && (buf < wsi->trunc_alloc || buf > (wsi->trunc_alloc + wsi->trunc_len + wsi->trunc_offset))) { - char dump[20]; - strncpy(dump, (char *)buf, sizeof(dump) - 1); - dump[sizeof(dump) - 1] = '\0'; - lwsl_err("** %p: Sending new %lu (%s), pending truncated ...\n" + lwsl_hexdump_level(LLL_ERR, buf, len); + lwsl_err("** %p: vh: %s, prot: %s, Sending new %lu, pending truncated ...\n" " It's illegal to do an lws_write outside of\n" " the writable callback: fix your code\n", - wsi, (unsigned long)len, dump); + wsi, wsi->vhost->name, wsi->protocol->name, + (unsigned long)len); assert(0); return -1; @@ -102,13 +118,20 @@ int lws_issue_raw(struct lws *wsi, unsigned char *buf, size_t len) n = lws_ssl_capable_write(wsi, buf, n); lws_latency(context, wsi, "send lws_issue_raw", n, n == len); + /* something got written, it can have been truncated now */ + wsi->could_have_pending = 1; + switch (n) { case LWS_SSL_CAPABLE_ERROR: /* we're going to close, let close know sends aren't possible */ wsi->socket_is_permanently_unusable = 1; return -1; case LWS_SSL_CAPABLE_MORE_SERVICE: - /* nothing got sent, not fatal, retry the whole thing later */ + /* + * nothing got sent, not fatal. Retry the whole thing later, + * ie, implying treat it was a truncated send so it gets + * retried + */ n = 0; break; } @@ -143,7 +166,7 @@ handle_truncated_send: /* * Newly truncated send. Buffer the remainder (it will get - * first priority next time the socket is writable) + * first priority next time the socket is writable). */ lwsl_debug("%p new partial sent %d from %lu total\n", wsi, n, (unsigned long)real_len); diff --git a/lib/plat/lws-plat-esp32.c b/lib/plat/lws-plat-esp32.c index 094959a0..b57c1c1a 100644 --- a/lib/plat/lws-plat-esp32.c +++ b/lib/plat/lws-plat-esp32.c @@ -79,6 +79,9 @@ lws_send_pipe_choked(struct lws *wsi) #endif int n; + /* the fact we checked implies we avoided back-to-back writes */ + wsi_eff->could_have_pending = 0; + /* treat the fact we got a truncated send pending as if we're choked */ if (wsi_eff->trunc_len) return 1; diff --git a/lib/plat/lws-plat-optee.c b/lib/plat/lws-plat-optee.c index 38037d20..2fdb81de 100644 --- a/lib/plat/lws-plat-optee.c +++ b/lib/plat/lws-plat-optee.c @@ -45,6 +45,19 @@ lws_get_random(struct lws_context *context, void *buf, int len) LWS_VISIBLE int lws_send_pipe_choked(struct lws *wsi) { + struct lws *wsi_eff = wsi; + +#if defined(LWS_WITH_HTTP2) + wsi_eff = lws_get_network_wsi(wsi); +#endif + + /* the fact we checked implies we avoided back-to-back writes */ + wsi_eff->could_have_pending = 0; + + /* treat the fact we got a truncated send pending as if we're choked */ + if (wsi_eff->trunc_len) + return 1; + #if 0 struct lws_pollfd fds; diff --git a/lib/plat/lws-plat-unix.c b/lib/plat/lws-plat-unix.c index 7bca1ac9..81c2d1be 100644 --- a/lib/plat/lws-plat-unix.c +++ b/lib/plat/lws-plat-unix.c @@ -93,6 +93,10 @@ lws_send_pipe_choked(struct lws *wsi) #if defined(LWS_WITH_HTTP2) wsi_eff = lws_get_network_wsi(wsi); #endif + + /* the fact we checked implies we avoided back-to-back writes */ + wsi_eff->could_have_pending = 0; + /* treat the fact we got a truncated send pending as if we're choked */ if (wsi_eff->trunc_len) return 1; diff --git a/lib/plat/lws-plat-win.c b/lib/plat/lws-plat-win.c index 5e2bab7f..1181184f 100644 --- a/lib/plat/lws-plat-win.c +++ b/lib/plat/lws-plat-win.c @@ -135,12 +135,19 @@ lws_get_random(struct lws_context *context, void *buf, int len) LWS_VISIBLE int lws_send_pipe_choked(struct lws *wsi) -{ +{ struct lws *wsi_eff = wsi; + +#if defined(LWS_WITH_HTTP2) + wsi_eff = lws_get_network_wsi(wsi); +#endif + /* the fact we checked implies we avoided back-to-back writes */ + wsi_eff->could_have_pending = 0; + /* treat the fact we got a truncated send pending as if we're choked */ - if (wsi->trunc_len) + if (wsi_eff->trunc_len) return 1; - return (int)wsi->sock_send_blocking; + return (int)wsi_eff->sock_send_blocking; } LWS_VISIBLE int diff --git a/lib/private-libwebsockets.h b/lib/private-libwebsockets.h index 22be0561..26eb90a7 100644 --- a/lib/private-libwebsockets.h +++ b/lib/private-libwebsockets.h @@ -515,6 +515,9 @@ enum lws_connection_states { _LSF_WEBSOCKET, LWSS_CGI = 17, + + LWSS_HTTP2_DEFERRING_ACTION = _LSF_CCB | 18 | + _LSF_POLLOUT, }; #define lws_state_is_ws(s) (!!(s & _LSF_WEBSOCKET)) @@ -1925,6 +1928,8 @@ struct lws { unsigned int rxflow_will_be_applied:1; unsigned int event_pipe:1; + unsigned int could_have_pending:1; /* detect back-to-back writes */ + unsigned int timer_active:1; #ifdef LWS_WITH_ACCESS_LOG diff --git a/lib/service.c b/lib/service.c index 68cbbad9..18d7cd33 100644 --- a/lib/service.c +++ b/lib/service.c @@ -25,7 +25,7 @@ static int lws_calllback_as_writeable(struct lws *wsi) { struct lws_context_per_thread *pt = &wsi->context->pt[(int)wsi->tsi]; - int n; + int n, m; lws_stats_atomic_bump(wsi->context, pt, LWSSTATS_C_WRITEABLE_CB, 1); #if defined(LWS_WITH_STATS) @@ -62,9 +62,11 @@ lws_calllback_as_writeable(struct lws *wsi) break; } - return user_callback_handle_rxflow(wsi->protocol->callback, + m = user_callback_handle_rxflow(wsi->protocol->callback, wsi, (enum lws_callback_reasons) n, wsi->user_space, NULL, 0); + + return m; } LWS_VISIBLE int @@ -95,6 +97,7 @@ lws_handle_POLLOUT_event(struct lws *wsi, struct lws_pollfd *pollfd) * If anything else sent first the protocol would be * corrupted. */ + wsi->could_have_pending = 0; /* clear back-to-back write detection */ if (wsi->trunc_len) { //lwsl_notice("%s: completing partial\n", __func__); if (lws_issue_raw(wsi, wsi->trunc_alloc + wsi->trunc_offset, @@ -464,6 +467,38 @@ user_service_go_again: goto next_child; } + if (w->state == LWSS_HTTP2_DEFERRING_ACTION) { + + /* + * we had to defer the http_action to the POLLOUT + * handler, because we know it will send something and + * only in the POLLOUT handler do we know for sure + * that there is no partial pending on the network wsi. + */ + + w->state = LWSS_HTTP2_ESTABLISHED; + + lwsl_info(" h2 action start...\n"); + n = lws_http_action(w); + lwsl_info(" h2 action result %d " + "(wsi->http.rx_content_remain %lld)\n", + n, w->http.rx_content_remain); + + /* + * Commonly we only managed to start a larger transfer + * that will complete asynchronously under its own wsi + * states. In those cases we will hear about + * END_STREAM going out in the POLLOUT handler. + */ + if (n || w->h2.send_END_STREAM) { + lwsl_info("closing stream after h2 action\n"); + lws_close_free_wsi(w, LWS_CLOSE_STATUS_NOSTATUS); + wa = &wsi->h2.child_list; + } + + goto next_child; + } + if (w->state == LWSS_HTTP_ISSUING_FILE) { ((volatile struct lws *)w)->leave_pollout_active = 0; diff --git a/plugins/protocol_lws_status.c b/plugins/protocol_lws_status.c index d9240cae..e456c8bd 100644 --- a/plugins/protocol_lws_status.c +++ b/plugins/protocol_lws_status.c @@ -62,18 +62,15 @@ struct per_vhost_data__lws_status { static void trigger_resend(struct per_vhost_data__lws_status *vhd) { - struct per_session_data__lws_status *pss = vhd->live_pss_list; - - while (pss) { + lws_start_foreach_ll(struct per_session_data__lws_status *, pss, + vhd->live_pss_list) { if (pss->walk == WALK_NONE) { pss->subsequent = 0; pss->walk_next = vhd->live_pss_list; pss->walk = WALK_INITIAL; } else pss->changed_partway = 1; - - pss = pss->next; - } + } lws_end_foreach_ll(pss, next); lws_callback_on_writable_all_protocol(vhd->context, vhd->protocol); } @@ -85,8 +82,7 @@ callback_lws_status(struct lws *wsi, enum lws_callback_reasons reason, void *user, void *in, size_t len) { struct per_session_data__lws_status *pss = - (struct per_session_data__lws_status *)user, - *pss1, *pss2; + (struct per_session_data__lws_status *)user; struct per_vhost_data__lws_status *vhd = (struct per_vhost_data__lws_status *) lws_protocol_vh_priv_get(lws_get_vhost(wsi), @@ -123,6 +119,7 @@ callback_lws_status(struct lws *wsi, enum lws_callback_reasons reason, strcpy(pss->user_agent, "unknown"); lws_hdr_copy(wsi, pss->user_agent, sizeof(pss->user_agent), WSI_TOKEN_HTTP_USER_AGENT); + trigger_resend(vhd); break; @@ -150,14 +147,14 @@ callback_lws_status(struct lws *wsi, enum lws_callback_reasons reason, pss->subsequent = 1; m = 0; - pss2 = vhd->live_pss_list; - while (pss2) { + lws_start_foreach_ll(struct per_session_data__lws_status *, + pss2, vhd->live_pss_list) { if (pss2 == pss->walk_next) { m = 1; break; } - pss2 = pss2->next; - } + } lws_end_foreach_ll(pss2, next); + if (!m) { /* our next guy went away */ pss->walk = WALK_FINAL; @@ -181,6 +178,7 @@ walk_final: n = LWS_WRITE_CONTINUATION; p += sprintf(p, "]}"); if (pss->changed_partway) { + pss->changed_partway = 0; pss->subsequent = 0; pss->walk_next = vhd->live_pss_list; pss->walk = WALK_INITIAL; @@ -207,22 +205,15 @@ walk_final: break; case LWS_CALLBACK_CLOSED: - pss1 = vhd->live_pss_list; - pss2 = NULL; - - while (pss1) { - if (pss1 == pss) { - if (pss2) - pss2->next = pss->next; - else - vhd->live_pss_list = pss->next; - + // lwsl_debug("****** LWS_CALLBACK_CLOSED\n"); + lws_start_foreach_llp(struct per_session_data__lws_status **, + ppss, vhd->live_pss_list) { + if (*ppss == pss) { + *ppss = pss->next; break; } + } lws_end_foreach_llp(ppss, next); - pss2 = pss1; - pss1 = pss1->next; - } trigger_resend(vhd); break; diff --git a/test-apps/test-server-http.c b/test-apps/test-server-http.c index 5ef640a1..0608c276 100644 --- a/test-apps/test-server-http.c +++ b/test-apps/test-server-http.c @@ -503,11 +503,9 @@ int callback_http(struct lws *wsi, enum lws_callback_reasons reason, void *user, if (n < 0) return 1; - n = lws_write(wsi, (unsigned char *)pss->result + LWS_PRE, - pss->result_len, LWS_WRITE_HTTP); - if (n < 0) - return 1; - goto try_to_reuse; + lws_callback_on_writable(wsi); + break; + case LWS_CALLBACK_HTTP_DROP_PROTOCOL: lwsl_debug("LWS_CALLBACK_HTTP_DROP_PROTOCOL\n"); @@ -526,7 +524,7 @@ int callback_http(struct lws *wsi, enum lws_callback_reasons reason, void *user, if (pss->client_finished) return -1; - if (!lws_get_child(wsi) && !pss->fop_fd) + if (!lws_get_child(wsi) && !pss->fop_fd && !pss->result_len) goto try_to_reuse; #ifndef LWS_NO_CLIENT @@ -561,6 +559,17 @@ int callback_http(struct lws *wsi, enum lws_callback_reasons reason, void *user, /* * we can send more of whatever it is we were sending */ + + if (pss->result_len) { + /* the result from the form */ + n = lws_write(wsi, (unsigned char *)pss->result + LWS_PRE, + pss->result_len, LWS_WRITE_HTTP); + pss->result_len = 0; + if (n < 0) + return 1; + goto try_to_reuse; + } + sent = 0; do { /* we'd like the send this much */