New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websockets: Add a client test again the autobahn testsuite + improvements #1534

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
3 participants
@Geod24
Contributor

Geod24 commented Jul 17, 2016

Here's a couple of improvements to the Websockets implementation.

First, a new 'test', which is not run by default, against the autobahn testsuite (see #1505 ).
Currently, it passes 517 out of 519 test cases (it blocks on the 518th, haven't investigated yet).

The other improvements are mostly cosmetic / performance: reduce the size of the Frame, document everything, provide a better encapsulation...

There's a change to the API, where 'send' with a message doesn't assume the data to be text anymore, and send didn't accept const(ubyte)[] - even though it was just forwarding to a const-accepting method.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Jul 19, 2016

Contributor

Fixed the failures. Now I only get the spurious ones.

Contributor

Geod24 commented Jul 19, 2016

Fixed the failures. Now I only get the spurious ones.

@mathias-lang-sociomantic

This comment has been minimized.

Show comment
Hide comment
Contributor

mathias-lang-sociomantic commented Jul 21, 2016

Ping @s-ludwig

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Jul 27, 2016

Contributor

Ping @s-ludwig

Contributor

Geod24 commented Jul 27, 2016

Ping @s-ludwig

Show outdated Hide outdated source/vibe/http/websockets.d
@@ -418,7 +425,8 @@ final class WebSocket {
Sends a message using an output stream.
Throws: WebSocketException if the connection is closed.
*/
void send(scope void delegate(scope OutgoingWebSocketMessage) sender, FrameOpcode frameOpcode = FrameOpcode.text)

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

There should be an additional compatibility wrapper here to avoid breaking code:

/// Compatibility overload - will be removed soon.
deprecated("Need to specify an explicit FrameOpcode.")
void send(scope void delegate(scope OutgoingWebSocketMessage) sender)
{
    send(sender, FrameOpcode.text);
}
@s-ludwig

s-ludwig Jul 27, 2016

Member

There should be an additional compatibility wrapper here to avoid breaking code:

/// Compatibility overload - will be removed soon.
deprecated("Need to specify an explicit FrameOpcode.")
void send(scope void delegate(scope OutgoingWebSocketMessage) sender)
{
    send(sender, FrameOpcode.text);
}
Show outdated Hide outdated source/vibe/http/websockets.d
* `false` if the current frame is the final one, `true` if a new frame
* was read.
*/
bool nextFrame()

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Nitpick: skipFrame() would be more consistent with skip(), which is used by other stream types.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Nitpick: skipFrame() would be more consistent with skip(), which is used by other stream types.

Show outdated Hide outdated source/vibe/http/websockets.d
@@ -697,108 +727,234 @@ final class IncomingWebSocketMessage : InputStream {
}
}
/// Magic string defined by the RFC for challenging the server during upgrade
private static immutable s_webSocketGuid = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11";

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Does static actually have an effect for private symbols?

@s-ludwig

s-ludwig Jul 27, 2016

Member

Does static actually have an effect for private symbols?

This comment has been minimized.

@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 27, 2016

Contributor

static immutable does: It means the variable is a CT-known constant that will end up in the data section. It is quite useful for arrays, for example:

static immutable data = [1, 2, 3, 4];

void process (immutable int[]) @nogc;

void main () @nogc
{
    process(data);
}

If one used enum here, main couldn't be @nogc.

@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 27, 2016

Contributor

static immutable does: It means the variable is a CT-known constant that will end up in the data section. It is quite useful for arrays, for example:

static immutable data = [1, 2, 3, 4];

void process (immutable int[]) @nogc;

void main () @nogc
{
    process(data);
}

If one used enum here, main couldn't be @nogc.

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

I always thought that this will always happen for immutable literals (at the least for string literals, not sure about arrays).

@s-ludwig

s-ludwig Jul 27, 2016

Member

I always thought that this will always happen for immutable literals (at the least for string literals, not sure about arrays).

This comment has been minimized.

@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 27, 2016

Contributor

Actually at module level immutable x = y still have the same behavior. Do you want me to revert that change ?

@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 27, 2016

Contributor

Actually at module level immutable x = y still have the same behavior. Do you want me to revert that change ?

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Nah, doesn't really matter, I was just wondering.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Nah, doesn't really matter, I was just wondering.

Show outdated Hide outdated source/vibe/http/websockets.d
- The 3 reserved bits RSV1, RSV2, RSV3
- The 4-bits opcode
*/
ubyte fin_rsv_opcode;

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

This could probably benefit from using bitfields instead of manual bit fiddling, but both are generally fine for me in this case.

@s-ludwig

s-ludwig Jul 27, 2016

Member

This could probably benefit from using bitfields instead of manual bit fiddling, but both are generally fine for me in this case.

Show outdated Hide outdated source/vibe/http/websockets.d
// RFC 6455, 5.2, 'Payload length': If 127, the following 8 bytes
// interpreted as a 64-bit unsigned integer (the most significant
// bit MUST be 0)
enforceEx!WebSocketException(!!(length >> 63),

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Should be just a single ! here.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Should be just a single ! here.

Show outdated Hide outdated travis-ci.sh
@@ -20,7 +20,9 @@ if [ ${BUILD_EXAMPLE=1} -eq 1 ]; then
fi
if [ ${RUN_TEST=1} -eq 1 ]; then
for ex in `\ls -1 tests/`; do
echo "[INFO] Running test $ex"
(cd tests/$ex && dub --compiler=$DC && dub clean)
if [ -r test/$ex/dub.{json,sdl} ]; then

This comment has been minimized.

@s-ludwig

s-ludwig Jul 27, 2016

Member

Bash complains for Travis-CI:

'[' -r test/vibe.core.net.1376/dub.json test/vibe.core.net.1376/dub.sdl ']'
./travis-ci.sh: line 23: [: test/vibe.core.net.1376/dub.json: binary operator expected

Should probably use [ -r test/$ex/dub.json} ] || [ -r test/$ex/dub.sdl ].

@s-ludwig

s-ludwig Jul 27, 2016

Member

Bash complains for Travis-CI:

'[' -r test/vibe.core.net.1376/dub.json test/vibe.core.net.1376/dub.sdl ']'
./travis-ci.sh: line 23: [: test/vibe.core.net.1376/dub.json: binary operator expected

Should probably use [ -r test/$ex/dub.json} ] || [ -r test/$ex/dub.sdl ].

This comment has been minimized.

@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 27, 2016

Contributor

Oops, good catch !

@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 27, 2016

Contributor

Oops, good catch !

Geod24 added some commits Jun 6, 2016

Add an application to test the client websockets implementation again…
…st autobahn

Autobahn (http://autobahn.ws/testsuite) is a comprehensive test suite
of the Websocket specifications.
This test won't be run by default as it requires way too much ressources
to put it into the CI, but can trivially be run manually by anyone wanting
to test the websockets implementation for conformity.
WebSocket: Deprecate default FrameOpcode parameter on send
We should not assume what the user is sending, as it varies from one application to another.
The default might as well be binary and would seem more sensible to some.
Websocket: Improve Frame.writeFrame code
Reduce the need for additional buffer, opening the way
for more memory improvements, and document what it is
doing.

Also corrects a bug with length == 65536, as it would be cast
to `ushort` which maximum size is 65535.

Finally, fixup documentation and type of FrameOpcode.
WebSocket: Improvements to Frame.readFrame
Reduce number of static buffers (3 -> 1)
Comment the internals
Only demask when the frame is masked (the previous code didn't yield incorrect data,
but needlessly iterated and assigned data to an array)
Check that the most significant bit of the 8 bytes length is 0.
@mathias-lang-sociomantic

This comment has been minimized.

Show comment
Hide comment
@mathias-lang-sociomantic

mathias-lang-sociomantic Nov 27, 2016

Contributor

Rebased on master and addressed your comments

Contributor

mathias-lang-sociomantic commented Nov 27, 2016

Rebased on master and addressed your comments

@s-ludwig s-ludwig added needs review and removed needs input labels Feb 15, 2017

@s-ludwig

Finally also got around to review this and all looks good. The only issue is that I accidentally made an overlapping change in the frame writing code. I'll manually rebase this and skip 4afc22b, which more or less does the same.

@s-ludwig s-ludwig removed the needs review label Jul 11, 2017

s-ludwig added a commit that referenced this pull request Jul 11, 2017

s-ludwig added a commit that referenced this pull request Jul 12, 2017

@s-ludwig s-ludwig closed this Jul 17, 2017

@mathias-lang-sociomantic

This comment has been minimized.

Show comment
Hide comment
@mathias-lang-sociomantic

mathias-lang-sociomantic Jul 17, 2017

Contributor

Oh, thank you very much ! Sorry this went off the radar a bit, I don't actively work on that project currently, since I had many issues (notably the fact that the bot would only survive a minute).
I should try again to see if everything is fixed now.

Contributor

mathias-lang-sociomantic commented Jul 17, 2017

Oh, thank you very much ! Sorry this went off the radar a bit, I don't actively work on that project currently, since I had many issues (notably the fact that the bot would only survive a minute).
I should try again to see if everything is fixed now.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 17, 2017

Member

Good that you mentioned it again! I actually remembered that I observed the same issue in one of my projects (it does an auto-reconnect anyway, so it didn't seem so important) and now pushed a fix for this: #1848

Member

s-ludwig commented Jul 17, 2017

Good that you mentioned it again! I actually remembered that I observed the same issue in one of my projects (it does an auto-reconnect anyway, so it didn't seem so important) and now pushed a fix for this: #1848

@Geod24 Geod24 deleted the Geod24:websockets branch Aug 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment