020a68 Broke binary socket frame

[lecajer]

Hi thanks for this great module. We use a Pushstream to send socket message to iOS app Our iOS app uses this library https://github.com/square/SocketRocket

When we upgraded our Pushstream module it stoped working on our iOS app It worked again without this commit 020a68824eae347974ce2b97d6a3009ba9461383

Is OpCodeBinaryFrame not supported ? It works fine with previous versions why stop supporting it ? Does someone know if pushstream module will support Binary frame again ?

We noticed that OpCodeTextFrame works fine with the latest commit of this repo.

Cheers.

[wandenberg]

Hi @Jeremlicious I blocked ant non tested frame, to avoid errors. Can you provide a valid binary frame and what result the client expect to receive? With that I can return to officially support the binary frame ;)

[lecajer]

That would be great! Does the following help you ?

[lecajer]

And this

TEXT CASE
0000   d4 ca 6d 22 dd f7 0c 4d e9 d2 c5 01 08 00 45 00  ..m"...M......E.
0010   00 5e 83 02 40 00 40 06 00 00 c0 a8 01 7d 36 4d  .^..@.@......}6M
0020   dc 35 f1 b1 00 50 80 44 42 3e 7c 20 7c ea 80 18  .5...P.DB>| |...
0030   10 0e d4 f8 00 00 01 01 08 0a 32 8b 9e 9b 13 04  ..........2.....
0040   f4 db 81 a4 ba 77 b8 d2 c1 7d 98 f2 98 03 c1 a2  .....w...}......
0050   df 55 98 e8 9a 55 d1 b6 98 5b b2 f2 9a 55 d1 b6  .U...U...[...U..
0060   98 57 82 f2 88 41 8c e4 8c 4e b2 af              .W...A...N..

BINARY CASE
0000   d4 ca 6d 22 dd f7 0c 4d e9 d2 c5 01 08 00 45 00  ..m"...M......E.
0010   00 5e 1d 3b 40 00 40 06 00 00 c0 a8 01 7d 36 4d  .^.;@.@......}6M
0020   dc 35 f1 ca 00 50 1d 72 ed de 56 96 c9 e5 80 18  .5...P.r..V.....
0030   10 0e d4 f8 00 00 01 01 08 0a 32 8c 1b 7a 13 05  ..........2..z..
0040   14 8e 82 a4 c3 1f f7 3f b8 15 d7 1f e1 6b 8e 4f  .......?.....k.O
0050   a6 3d d7 05 e3 3d 9e 5b e1 33 fd 1f e3 3d 9e 5b  .=...=.[.3...=.[
0060   e1 3f cd 1f f1 29 c3 09 f5 26 fd 42              .?...)...&.B

[wandenberg]

@Jeremlicious the module did not have support to send binary frames, this was why I remove the support to receive binary frames, since it have to forward as a text frame what didn't seems right. Can you provide a small code which used to work on your application? I will try to use it as a automated test to the module if I can safely add support to binary frames.

[lecajer]

@wandenberg thanks for wanting to officially support binary frame. I can confirm that we've been using nginx-push-stream-module since october 2014 with binaryframe on an iOS app. Do you need a code that would be in a iOS app client or on the server that sends the messages ?

If iOS @angu spotted the issue and might be able to provide more code than what I posted above.

[wandenberg]

@Jeremlicious Are you sure that you have sent binary message using websocket at some point? Since the very first commit that added support to websocket the module do the following check

if (cf->websocket_allow_publish && (frame.opcode == NGX_HTTP_PUSH_STREAM_WEBSOCKET_TEXT_OPCODE)) {

May be you have POST the binary message and received it using websocket and got confused now ?!

[angu]

@wandenberg The scenario is a bit different. From the client side we are sending an empty message to the socket just to say that we were listening. The "empty" message is sent as a binary message, so in the old implementation the server just silently discards it and then we get text messages from the socket to the client (the communication is unilateral, we only get messages from the socket to the client). With the new implementation the binary message is discarded and the socket is forced to close, so the client stops working. Honestly I don't think that the binary support is necessary, since the server never handles the message from the client. I also want to check if the "standard" ping -pong message is enough to not close the socket, so the client can avoid to send any message at all.

I hope this clarifies the issue.

[wandenberg]

Hi @angu, yes this explanation clarify the issue. The default ping pong should be enough to keep the connection open, this is why it was created on the protocol. Unfortunately, the hack you have used will no longer work, and does not make sense to add binary support to the module.

[lecajer]

Hi guys, thanks for clarifying the issue. It would be good that the latest version of Pushstream module still support old implementation so we can upgrade the server safely. We keep the backward compatibility with our old iOS relases. @wandenberg is there a way to support backward compatibility in our case ? Maybe an option in the config ? In parrallel we will implement the best practice with socket on our upcomming release but some people might not update. @augu we'll follow up on this.

[wandenberg]

@Jeremlicious, the effort to support this compatibility adding an option will not worth. You can remove the code bellow on the file ngx_http_push_stream_module_websocket.c on you build. Test it please.

                if (
                    (ctx->frame->opcode != NGX_HTTP_PUSH_STREAM_WEBSOCKET_TEXT_OPCODE) &&
                    (ctx->frame->opcode != NGX_HTTP_PUSH_STREAM_WEBSOCKET_CLOSE_OPCODE) &&
                    (ctx->frame->opcode != NGX_HTTP_PUSH_STREAM_WEBSOCKET_PING_OPCODE) &&
                    (ctx->frame->opcode != NGX_HTTP_PUSH_STREAM_WEBSOCKET_PONG_OPCODE)
                   ) {
                    goto close;
                }

[lecajer]

Ok understood. Thanks again for supporting this library.