ws: fix and extend CURLWS_CONT handling #16687
Conversation
viscruocco
force-pushed
the
websocket-fix-fragmentation
branch
from
798ea63 to
11403da
Compare
|
On the terminology: The term fragmentation and fragments in the WebSocket spec seems to be a verb when discussing using multiple frames for a single message. I believe I have tried to stick to the terms frame and message for the two fundamental building WebSocket "blocks" while a fragment is a partial (or complete) piece of a WebSocket frame. In order to properly be able to stream the sending and receiving of the potentially very large WebSocket frames. IOW what you call chunks here. It might be a good idea to just properly change and use the word chunk for this in our API documentation going forward, to make it less confusing and possibly more consistent. |
|
Thoughts on this @roberte777 ? |
|
Analysis of PR #16687 at 11403da4: Test 2311 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 26 different CI jobs (the link just goes to one of them). Test 818 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Generated by Testclutch |
I see handling for:
So I believe this pull request implements everything required for handling fragmented websocket messages. I also think this interpretation of bytesLeft and CURLWS_CONT is correct, and it matches my original presumptions when using CURL for websockets as well. I think I misunderstood what was being asked of me when adding the change to only unset CURLWS_CONT on the last chunk as opposed to having it not be set on the entire last frame. I apologize for any issues with my changes. Thanks to @viscruocco for getting this completed, I think this is a much better implementation. The unit test I made will need to be updated to properly reflect these changes. In addition, further testing for fragmented messages is a good idea. |
I've already tried to unify the existing occurrences of the terms in #16118, but I'm afraid lots of explanations are still pretty short or scattered all over the place.
I'm trying to adapt the existing test and agree that further test are direly needed. I'm having a hard time getting into the test setup though; so far I've just tested the integration in our own client application. Are there any guides how to build and run the libcurl tests locally on Linux?
IMHO, there isn't anything to apologize for! The changes you contributed were a huge step in the right direction and well executed! The behavior of CURLWS_CONT has just proven to be notoriously hard to communicate and implement, I think. |
Yeah this was tough. I did get it working though. The first step is to make sure you have all the required dependencies from the tests README. In addition, I used the Additionally, I needed to add |
Follow-up to fa3d1e7 Add test 2311 to verify Closes curl#16687
described in WebSocket continuation flag inverted for received fragments #15298.
The work in #16512 / fa3d1e7 set out to address the handling of the CURLWS_CONT
flag, but did not modify the handling of the accompanying CURLWS_TEXT and
CURLWS_BINARY flag. This would probably be sufficient to allow an application to
correctly implement fragmented message handling. With regard to fragmented
messages it also seems compatible with the existing interface documentation,
although the documentation appears underspecified and allows for varying
interpretations whether the CURLWS_TEXT/CURLWS_BINARY should be present in
following fragments or not (as described in #15298).
Unfortunately #16512 / fa3d1e7 also modified the CURLWS_CONT flag handling for
partially processed frames occuring for non-blocking interfaces like
curl_ws_send()/curl_ws_recv(). The new behavior seems unhelpful, may breakexisting applications and is in violation of the documentation.
Terminology
The discussion in #16503 shows that the terminology around fragmented WebSocket
messages is not very clear. Hence, let me fist pin down the most important terms
before carrying on:
Frame:
A "packet" of data encoded with the header and payload structure as described in
RFC 6455 Section 5 "Data Framing".
Message:
An abstract unit of information. From RFC 6455 Section 1.2 "Protocol Overview":
According to RFC 6455 Section 5.4 "Fragmentation"
a message delivered in a single frame is called an "unfragmented message".
In contrast, a message split over multiple frames is called a "fragmented message".
Fragment:
According to RFC 6455 Section 5.4 "Fragmentation" this refers to the
part of a "fragmented message" which is delivered within a single frame.
Strictly spoken the term "fragment" only refers to payload data, while the
frame carrying a "fragment" does not have a special name. For simplicity
though, in some context the full frame might be called a "fragment" itself.
(I'll try to avoid this looser wording here.)
Chunk:
When
curl_ws_send()/curl_ws_recv()are used in conjunction with theCURLWS_OFFSETflag or are not able to process enough data in a single call, then they will
only consume/produce a piece of the data and indicate this via their
sent/meta->bytesleftoutput parameter. The libcurl documentation does not introduce anofficial term for this slice of partial data, but I will refer to it as "chunk"
to avoid confusion with the term "fragment".
The difference between fragments and "chunks" seemed to be a clear
source of confusion in the discussion about Expose FIN bit via curl_ws_meta #16503.
CONT:
There is a notable difference between the usage of the term CONT in the WebSocket RFC and libcurl.
RFC 6455 uses CONT with a semantic of "this is the continuation of a prior fragment".
Libcurl instead uses CONT with a semantic of "this will be continued in a following fragment".
Unfortunately it's too late to amend this naming since WebSocket support is not
experimental anymore, so we'll have to live with it.
Hence, the RFC marks the frames of a fragmented message in the following way.
(WSBIT_OPCODE_TEXT or WSBIT_OPCODE_BIN) and the WSBIT_FIN bit is cleared.
the OPCODE is set to WSBIT_OPCODE_CONT and the WSBIT_FIN bit is cleared.
WSBIT_FIN bit is set.
While libcurl marks the frames of a fragmented message in the following way.
CURLWS_TEXT/CURLWS_BINARY flag.
does not specify whether the CURLWS_TEXT/CURLWS_BINARY flag is set or cleared.
does not specify whether the CURLWS_TEXT/CURLWS_BINARY flag is set or cleared.
Problem Statement
The yet unspecified portion whether the CURLWS_TEXT/CURLWS_BINARY flag is set or cleared
in the intermediate frames and final frame is one of two motivators behind this PR.
The other - more important - motivator is to the adjust the way the CURLWS_CONT
flag is handled for chunks.
Before #16512 / fa3d1e7 the CURLWS_CONT flag would never show up for
unfragmented messages, because it was solely based on the frames opcode.
This was changed (and arguably broken) in #16512 / fa3d1e7.
The discussion in #16503, which was probably influenced by a confusion of fragments
and chunks, prompted that in #16512 / fa3d1e7 the CURLWS_CONT flag is now set
on all but the final chunks of a frame. Importantly this includes (but is
not limited to) the case where an unfragmented message is delivered in a single
frame but received as several chunks. Contrary to the old behavior this will
now set the CURLWS_CONT flag.
For reference: the currently documented method to check whether a non-blocking
call was able to process a frame as a single chunk is to evaluate the
sentoutput parameter of
curl_ws_send()respectively themeta->bytesleftoutput parameter of
curl_ws_recv(). In turn the current documentation does notmention any other use for the CURLWS_CONT flag than the fragmented message
signalling I have described previously.
The change may break existing application logic which interpret the occurence
of CURLWS_CONT as a sign that more frames with subsequent message fragments will
follow. They could detect the discrepancy by explicitly paying attention the
presence of the CURLWS_CONT flag in the last chunk of a frame (identified
through the value of
sent/meta->bytesleft) but it is very unlikely that anyexisting implementation would bother to verify this. Instead, applications
may just read the flags on the first chunk and assume they will either remain
the same or be completely unset for all subsequent chunks, since the
documentation did not specify anything else.
Besides from potentially breaking current applications and violating the
documentation, the newly introduced behavior does not offer an additional
benefit over the existing method to check the
sent/meta->bytesleftvalue.On the contrary: checking
sent/meta->bytesleftwould be absolutely mandatoryto identify the last chunk of the frame, which would then carry the CURLWS_CONT
flag describing the whole frame. In this last chunk the CURLWS_CONT flag could
either be set to indicate that more fragments will follow in the next frames or be
cleared to indicate that the message is complete.
Proposed Solution
This PR attempts to implement and document the behavior as follows:
For consecutive frames of a fragmented message, the behavior shall be as
already perfectly summarized in #15298:
For consecutive data chunks the flags shall be guaranteed to be identical.
Hence, the CURLWS_CONT flag shall either be present in all chunks of a frame
(for first/intermediate frame of a fragmented message) or none of the chunks of
a frame (for final frame or unfragmented message).
Implementation Notes
The issue with CURLWS_CONT for chunks can probably be fixed by a one-liner change:
To set the CURLWS_TEXT/CURLWS_BINARY flag on the intermediate and final fragments,
it is necessary to store this information in the decoder struct during header
parsing of the first fragment and carry the information over to the following fragments.
Hence, the addition of the
dec->cont_flagsfield.Ultimately the existing code for processing of the first header byte (fin bit,
reserved bits and opcode) had become pretty complex, so I replaced
ws_frame_op2flags()/ws_frame_flags2op()with the more integratedws_frame_firstbyte2flags()/ws_frame_flags2firstbyte(). This easedthe necessary adaptions and should also improve future maintainability a lot.