noisyboiler / wampy

Websocket RPC and Pub/Sub for Python applications and microservices
Mozilla Public License 2.0
127 stars 24 forks source link

Support CLOSE messages; #60

Closed tortoisedoc closed 6 years ago

tortoisedoc commented 6 years ago

do not cause the client to crash. For example, Thruway server initiates closing of connection from server side with CLOSE message after GOODBYE.

noisyboiler commented 6 years ago

i'm ashamed that wampy does not support this already! let's get this in asap. thanks @tortoisedoc i'll look at it asap.

tortoisedoc commented 6 years ago

Okay. Are you going to merge the PR eventually?

noisyboiler commented 6 years ago

I'll have some time this evening @tortoisedoc

noisyboiler commented 6 years ago

hi @tortoisedoc It would be great to understand how you're using wampy. can you explain in a comment here? I'm not aware of the server you mention and it would be interesting to here what project you are using wampy on. also, please add yourself to CONTRIBUTORS.txt. thank you!

tortoisedoc commented 6 years ago

@noisyboiler for unittesting. Update : we are using symfony 3.4 with thruway as server. Sorry didnt realize to post some specs, too much in a hurry :)

tortoisedoc commented 6 years ago

I pushed another change; unicode conversion needed some handling as the CLOSE message has a 2 bytes payload which is a bytearray and hence has no encode / decode function.

See here : https://github.com/noisyboiler/wampy/pull/60/commits/08eb04d582af429e2d25d95a3412c05c2d45f9b3#diff-d8f8d4835f653e4ecf2216eb14e71c0bR90

noisyboiler commented 6 years ago

we need to sort the test fails @tortoisedoc and the comments i left you. i want this feature in, so do you want me to help make this PR compliant? regardless, you'll have the credit in the release notes :) thanks @tortoisedoc

noisyboiler commented 6 years ago

sorry, you did address them. one of us needs to remove that backslash though, sorry. since we spoke I've updated CONTRIBUTORS. I'm happy to close this off when i next get an hour or so. and i'll release it to PyPi straight away for you.

Please keep contributing! :)

tortoisedoc commented 6 years ago

Hi; ok great; so is the build fixing now on your or my side?

noisyboiler commented 6 years ago

I'll figure it out tonight if i can

noisyboiler commented 6 years ago

sorry to say that the test fails look due to these changes @tortoisedoc there is a unicode bug now + the change to the Result message has caused a lot of fails i think. have you run the tests locally? I'm going to create my own branch that mirrors yours to see if i can come up with some suggested fixes for you.

noisyboiler commented 6 years ago

ah, wampy only supports the kwargs in a WAMP result message, so return self.yield_args isn't going to work. let me see how much work it is to support that - as clearly wampy should as it's part of the protocol.

tortoisedoc commented 6 years ago

Sorry for breaking the build :). It started misbehaving after the utf-8 changes; before that it was green. As for the tests, I couldnt run them locally.

tortoisedoc commented 6 years ago

It also appears that warning messages are not properly reported back to the caller (for example if you forget a parameter from a rpc call)

noisyboiler commented 6 years ago

i'm refactoring the websocket and it will be much clearer very soon how to add your feature and also test it. bear with me @tortoisedoc .

noisyboiler commented 6 years ago

@tortoisedoc see https://github.com/noisyboiler/wampy/releases/tag/v0.9.16 and the latest release to PyPi for a much neater way now to introduce the CLOSE message. changing the result message I'm pretty sure is not required to support CLOSE - but please exaplin to me why if you believe so. but I may look into this myself now. Thanks @tortoisedoc

noisyboiler commented 6 years ago

Hi again @tortoisedoc thanks for the heads-up about this missing feature. the websocket implementation in wampy was just too unclear and buggy for you to add CLOSE, so sorry about that. I refactored it and fixed a number of bugs I exposed and then added the CLOSE message here: https://github.com/noisyboiler/wampy/pull/63 I also added you to the contributors list because of brining it to my attention and your work here. Thanks again. Simon