Skip to content

Commit 3957c72

Browse files
mathieucarboumathieucarbou
authored
Merge pull request #388 from ESP32Async/fix/385
Refactor the code handling WS data event to remove code duplication and fix the usages of pinfo->opcode in examples
2 parents e3fd7ad + fad8ca3 commit 3957c72

4 files changed

Lines changed: 92 additions & 72 deletions

File tree

‎examples/WebSocket/WebSocket.ino‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@ static const char *htmlContent PROGMEM = R"(
4949
ws.send(message);
5050
console.log("WebSocket sent: " + message);
5151
}
52+
setInterval(function() {
53+
if (ws.readyState === WebSocket.OPEN) {
54+
ws.send("msg from browser");
55+
}
56+
}, 1000);
5257
</script>
5358
</body>
5459
</html>
@@ -104,12 +109,13 @@ void setup() {
104109
} else if (type == WS_EVT_DATA) {
105110
AwsFrameInfo *info = (AwsFrameInfo *)arg;
106111
Serial.printf(
107-
"index: %" PRIu64 ", len: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", framelen: %d\n", info->index, info->len, info->final, info->opcode, len
112+
"index: %" PRIu64 ", len: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", framelen: %d\n", info->index, info->len, info->final,
113+
info->message_opcode, len
108114
);
109115

110116
// complete frame
111117
if (info->final && info->index == 0 && info->len == len) {
112-
if (info->opcode == WS_TEXT) {
118+
if (info->message_opcode == WS_TEXT) {
113119
Serial.printf("ws text: %s\n", (char *)data);
114120
client->ping();
115121
}

‎idf_component_examples/websocket/main/main.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ void setup() {
5454
AwsFrameInfo *info = (AwsFrameInfo *)arg;
5555
String msg = "";
5656
if (info->final && info->index == 0 && info->len == len) {
57-
if (info->opcode == WS_TEXT) {
57+
if (info->message_opcode == WS_TEXT) {
5858
Serial.printf("ws text: %s\n", (char *)data);
5959
}
6060
}

‎src/AsyncWebSocket.cpp‎

Lines changed: 80 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,9 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
565565
uint8_t *data = (uint8_t *)pbuf;
566566

567567
while (plen > 0) {
568-
async_ws_log_v("WS[%" PRIu32 "]: _onData: plen=%" PRIu32 ", _pstate=%" PRIu8 ", _status=%" PRIu8, _clientId, plen, _pstate, static_cast<uint8_t>(_status));
568+
async_ws_log_v(
569+
"WS[%" PRIu32 "] _onData: plen: %" PRIu32 ", _pstate: %" PRIu8 ", _status: %" PRIu8, _clientId, plen, _pstate, static_cast<uint8_t>(_status)
570+
);
569571

570572
if (_pstate == STATE_FRAME_START) {
571573
const uint8_t *fdata = data;
@@ -593,7 +595,7 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
593595
}
594596

595597
async_ws_log_v(
596-
"WS[%" PRIu32 "]: _pinfo: index: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", masked: %" PRIu8 ", len: %" PRIu64, _clientId, _pinfo.index,
598+
"WS[%" PRIu32 "] _pinfo: index: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", masked: %" PRIu8 ", len: %" PRIu64, _clientId, _pinfo.index,
597599
_pinfo.final, _pinfo.opcode, _pinfo.masked, _pinfo.len
598600
);
599601

@@ -606,7 +608,7 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
606608
if (plen == 0) {
607609
// Safari close frame edge case: masked bit set but no mask data
608610
if (_pinfo.opcode == WS_DISCONNECT) {
609-
async_ws_log_v("WS[%" PRIu32 "]: close frame with incomplete mask, treating as unmasked", _clientId);
611+
async_ws_log_v("WS[%" PRIu32 "] close frame with incomplete mask, treating as unmasked", _clientId);
610612
_pinfo.masked = 0;
611613
_pinfo.index = 0;
612614
_pinfo.len = 0;
@@ -616,7 +618,7 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
616618

617619
//wait for more data
618620
_pstate = STATE_FRAME_MASK;
619-
async_ws_log_v("WS[%" PRIu32 "]: waiting for more mask data: read=%" PRIu8 "/4", _clientId, _pinfo.masked - 1);
621+
async_ws_log_v("WS[%" PRIu32 "] waiting for more mask data: read: %" PRIu8 "/4", _clientId, _pinfo.masked - 1);
620622
return;
621623
}
622624

@@ -632,7 +634,7 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
632634

633635
// restore masked to 1 for backward compatibility
634636
if (_pinfo.masked >= 5) {
635-
async_ws_log_v("WS[%" PRIu32 "]: mask read complete", _clientId);
637+
async_ws_log_v("WS[%" PRIu32 "] mask read complete", _clientId);
636638
_pinfo.masked = 1;
637639
}
638640

@@ -644,54 +646,37 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
644646
}
645647
}
646648

647-
if ((datalen + _pinfo.index) < _pinfo.len) {
648-
_pstate = STATE_FRAME_DATA;
649-
async_ws_log_v("WS[%" PRIu32 "]: processing next fragment index=%" PRIu64 ", len=%" PRIu32 "", _clientId, _pinfo.index, (uint32_t)datalen);
650-
651-
if (_pinfo.index == 0) {
652-
if (_pinfo.opcode) {
653-
_pinfo.message_opcode = _pinfo.opcode;
654-
_pinfo.num = 0;
655-
}
649+
if (_pinfo.index == 0) { // first fragment of the frame
650+
// init message_opcode for this frame
651+
// note: For next WS_CONTINUATION frames, they have opcode 0, so message_opcode will stay like the first frame
652+
if (_pinfo.opcode == WS_TEXT || _pinfo.opcode == WS_BINARY) {
653+
_pinfo.message_opcode = _pinfo.opcode;
656654
}
655+
// init frame number to 0 if only 1 frame or if this is the first frame of a fragmented message
656+
if (_pinfo.final || datalen < _pinfo.len) {
657+
_pinfo.num = 0;
658+
}
659+
}
657660

658-
if (datalen > 0) {
659-
// ------------------------------------------------------------
660-
// Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
661-
// Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
662-
// The initial design of the library was doing a backup of the byte following the data buffer because the client code
663-
// was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
664-
// This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
665-
// So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
666-
// ------------------------------------------------------------
667-
uint8_t *copy = (uint8_t *)malloc(datalen + 1);
668-
669-
if (copy == NULL) {
670-
async_ws_log_e("Failed to allocate");
671-
_status = WS_DISCONNECTED;
672-
if (_client) {
673-
_client->abort();
674-
}
675-
return;
676-
}
677-
678-
memcpy(copy, data, datalen);
679-
copy[datalen] = 0;
680-
681-
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
661+
if ((datalen + _pinfo.index) < _pinfo.len) { // more fragments to read for this frame
662+
_pstate = STATE_FRAME_DATA;
682663

683-
free(copy);
664+
if (datalen > 0) {
665+
async_ws_log_v(
666+
"WS[%" PRIu32 "] processing next fragment of %s frame %" PRIu32 ", index: %" PRIu64 ", len: %" PRIu32 "", _clientId,
667+
(_pinfo.message_opcode == WS_TEXT) ? "text" : "binary", _pinfo.num, _pinfo.index, (uint32_t)datalen
668+
);
669+
_handleDataEvent(data, datalen, datalen == plen); // datalen == plen means that we are processing the last part of the current TCP packet
684670
}
685671

686672
// track index for next fragment
687673
_pinfo.index += datalen;
688674

689-
} else if ((datalen + _pinfo.index) == _pinfo.len) {
675+
} else if ((datalen + _pinfo.index) == _pinfo.len) { // this is the last fragment for this frame
690676
_pstate = STATE_FRAME_START;
691-
async_ws_log_v("WS[%" PRIu32 "]: processing final fragment index=%" PRIu64 ", len=%" PRIu32 "", _clientId, _pinfo.index, (uint32_t)datalen);
692677

693678
if (_pinfo.opcode == WS_DISCONNECT) {
694-
async_ws_log_v("WS[%" PRIu32 "]: processing disconnect", _clientId);
679+
async_ws_log_v("WS[%" PRIu32 "] processing disconnect", _clientId);
695680

696681
if (datalen) {
697682
uint16_t reasonCode = (uint16_t)(data[0] << 8) + data[1];
@@ -714,49 +699,29 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
714699
}
715700

716701
} else if (_pinfo.opcode == WS_PING) {
717-
async_ws_log_v("WS[%" PRIu32 "]: processing ping", _clientId);
702+
async_ws_log_v("WS[%" PRIu32 "] processing ping", _clientId);
718703
_server->_handleEvent(this, WS_EVT_PING, NULL, NULL, 0);
719704
_queueControl(WS_PONG, data, datalen);
720705

721706
} else if (_pinfo.opcode == WS_PONG) {
722-
async_ws_log_v("WS[%" PRIu32 "]: processing pong", _clientId);
707+
async_ws_log_v("WS[%" PRIu32 "] processing pong", _clientId);
723708
if (datalen != AWSC_PING_PAYLOAD_LEN || memcmp(AWSC_PING_PAYLOAD, data, AWSC_PING_PAYLOAD_LEN) != 0) {
724709
_server->_handleEvent(this, WS_EVT_PONG, NULL, NULL, 0);
725710
}
726711

727712
} else if (_pinfo.opcode < WS_DISCONNECT) { // continuation or text/binary frame
728-
async_ws_log_v("WS[%" PRIu32 "]: processing data frame num=%" PRIu32 "", _clientId, _pinfo.num);
729-
730-
// ------------------------------------------------------------
731-
// Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
732-
// Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
733-
// The initial design of the library was doing a backup of the byte following the data buffer because the client code
734-
// was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
735-
// This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
736-
// So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
737-
// ------------------------------------------------------------
738-
uint8_t *copy = (uint8_t *)malloc(datalen + 1);
739-
740-
if (copy == NULL) {
741-
async_ws_log_e("Failed to allocate");
742-
_status = WS_DISCONNECTED;
743-
if (_client) {
744-
_client->abort();
745-
}
746-
return;
747-
}
713+
async_ws_log_v(
714+
"WS[%" PRIu32 "] processing final fragment of %s frame %" PRIu32 ", index: %" PRIu64 ", len: %" PRIu32 "", _clientId,
715+
(_pinfo.message_opcode == WS_TEXT) ? "text" : "binary", _pinfo.num, _pinfo.index, (uint32_t)datalen
716+
);
748717

749-
memcpy(copy, data, datalen);
750-
copy[datalen] = 0;
718+
_handleDataEvent(data, datalen, datalen == plen); // datalen == plen means that we are processing the last part of the current TCP packet
751719

752-
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
753720
if (_pinfo.final) {
754721
_pinfo.num = 0;
755722
} else {
756723
_pinfo.num += 1;
757724
}
758-
759-
free(copy);
760725
}
761726

762727
} else {
@@ -778,6 +743,52 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
778743
}
779744
}
780745

746+
void AsyncWebSocketClient::_handleDataEvent(uint8_t *data, size_t len, bool endOfPaquet) {
747+
// ------------------------------------------------------------
748+
// Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
749+
// Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
750+
// The initial design of the library was doing a backup of the byte following the data buffer because the client code
751+
// was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
752+
// This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
753+
// So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
754+
// ------------------------------------------------------------
755+
//
756+
// Optimization notes:
757+
//
758+
// 1) opcodes
759+
//
760+
// - info->opcode stores the current WS frame type (binary, text, continuation)
761+
// - info->message_opcode stores the WS frame type of the first frame of the message, which is used for fragmented messages to know the message type when processing subsequent frame with opcode 0 (continuation)
762+
// So we can use info->message_opcode to avoid copying the data for non-text frames, and only copy the data for text frames when we need to add a null terminator for client code convenience.
763+
//
764+
// 2) data copy vs data backup/restore
765+
// - endOfPaquet: is true when datalen == plen. plen is the remaining bytes in the current TCP packet, so if datalen == plen, it means that we are processing the last part of the current TCP packet.
766+
// In that case, we have to copy since we cannot backup/restore the byte after the data buffer.
767+
// Otherwise we can backup the byte and restore since we know that the byte after is owned by the current TCP packet (same pointer).
768+
if (_pinfo.message_opcode == WS_TEXT) {
769+
if (endOfPaquet) {
770+
std::unique_ptr<uint8_t[]> copy(new (std::nothrow) uint8_t[len + 1]());
771+
if (copy) {
772+
memcpy(copy.get(), data, len);
773+
copy[len] = 0;
774+
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy.get(), len);
775+
} else {
776+
async_ws_log_e("Failed to allocate");
777+
if (_client) {
778+
_client->abort();
779+
}
780+
}
781+
} else {
782+
uint8_t backup = data[len];
783+
data[len] = 0;
784+
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, len);
785+
data[len] = backup;
786+
}
787+
} else {
788+
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, len);
789+
}
790+
}
791+
781792
size_t AsyncWebSocketClient::printf(const char *format, ...) {
782793
va_list arg;
783794
va_start(arg, format);

‎src/AsyncWebSocket.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,9 @@ class AsyncWebSocketClient {
236236
void _runQueue();
237237
void _clearQueue();
238238

239+
// this function is called when a text message is received, in order to copy the buffer and place a null terminator at the end of the buffer for easier handling of text messages.
240+
void _handleDataEvent(uint8_t *data, size_t len, bool endOfPaquet);
241+
239242
public:
240243
void *_tempObject;
241244

0 commit comments

Comments
 (0)