Fix connection linger for WebSTOMP

[csicar]

Proposed Changes

Resolve issue #6737 by delaying closing the websocket connection to happen after the ERROR frame is sent.

Types of Changes

• Bug fix (non-breaking change which fixes issue Web STOMP: ERROR frame does not reach the client when subscribing to a non-existent target #6737)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)
• Build system and/or CI

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• I have added tests that prove my fix is effective or that my feature works
• All tests pass locally with my changes
• If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
• If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

Cause of the issue seems to be that process_request enqueues an ERROR frame, but handle_data1 closes the connection immediately. By enqueuing a message close_websocket instead of closing the connection directly, we avoid this problem

[pivotal-cla]

@csicar Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@csicar Thank you for signing the Contributor License Agreement!

[lukebakken]

Thanks! I'll have some small changes to push to your branch / fork.

[lukebakken]

The new test fails:

cowboy_websocket_SUITE:sub_non_existent failed on line 192
Reason: {badmatch,{close,{1006,"Connection closed abnormally"}}}
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Testing deps.rabbitmq_web_stomp.cowboy_websocket_SUITE: *** FAILED test case 5 of 11 ***

I can reproduce this locally by running this command:

make -C deps/rabbitmq_web_stomp ct-cowboy_websocket t=integration:sub_non_existent

Sometimes the test times out when I run it locally.

I'm looking into it.

[csicar]

That's surprising...
I tested with make tests and that worked in my machine. Seems like I get the same result now. I must have changed something during cleanup.

[lukebakken]

I took some time to review the code that deals with WebSTOMP. It would be great if this fix could be made to work, because the "correct" (i.e. better use of cowboy_websocket) would involve quite a bit of rewriting.

[csicar]

I think I finally found what the problems are.

The websocket client rfc6455_client, which is used for testing, has two places for receive ing messages:

1. In loop (which is started by init) where a TCP close message is interpreted as an error resulting in the error seen in my test: "Connection closed abnormally"
2. In recv (which is called by recv_raw) where a close message is handled as a normal message and returned to the caller of recv

The server sends the correct messages in the correct sequence (which I confirmed with wireshark), but the websocket client does not make it accessible to the test. (Or I'm using it incorrectly)

Why the test blocked: Waiting for rfc6455_client:close(WS), when the server already closed the connection, will never finish. I suspect that this caused by how it is implemented:

close(WS, WsReason) ->
    WS ! {close, WsReason},
    receive
        {rfc6455, close, WS, R} ->
            {close, R}
    end.

When the connection is already closed, the client will never receive a close message; blocking indefinitely.

Also: There seem to be problems with how the tests are cleaned up: When cancelling a test, some erlang processes remain running (Confirmed by running ps aux | grep ct_run after make test finished)

[csicar]

because the "correct" (i.e. better use of cowboy_websocket) would involve quite a bit of rewriting.

Yeah.. I came to the same conclusion

[lukebakken]

Thank you!

[csicar]

Thanks for merging!