Skip to content
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

WebsocketApp does not identify callback arguments correctly on 0.51.0 #467

Closed
swedishmike opened this issue Aug 19, 2018 · 11 comments
Closed

Comments

@swedishmike
Copy link

swedishmike commented Aug 19, 2018

The issue 463 referenced that version 0.49.0 seemed to break callback arguments. Version 0.51.0 was released with a reverse of 442 to fix this.

I've tried this version with the certstream library and the issue is still there as far as I can gather. If I revert to version 0.48.0 it all works again. For my own usage I'm quite happy to use requirements.txt to make sure that version 0.48.0 is used but this might cause issues for others as well?

I use the following example from the Certstream README to replicate the issue.

import logging
import sys
import datetime
import certstream

def print_callback(message, context):
    logging.debug("Message -> {}".format(message))

    if message['message_type'] == "heartbeat":
        return

    if message['message_type'] == "certificate_update":
        all_domains = message['data']['leaf_cert']['all_domains']

        if len(all_domains) == 0:
            domain = "NULL"
        else:
            domain = all_domains[0]

        sys.stdout.write(u"[{}] {} (SAN: {})\n".format(datetime.datetime.now().strftime('%m/%d/%y %H:%M:%S'), domain, ", ".join(message['data']['leaf_cert']['all_domains'][1:])))
        sys.stdout.flush()

logging.basicConfig(format='[%(levelname)s:%(name)s] %(asctime)s - %(message)s', level=logging.INFO)

certstream.listen_for_events(print_callback)

Here are some examples of the errors that are being reported:

[ERROR:websocket] 2018-08-20 07:26:53,347 - error from callback <bound method CertStreamClient._on_open of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_open() takes exactly 2 arguments (1 given)
[ERROR:websocket] 2018-08-20 07:26:53,524 - error from callback <bound method CertStreamClient._on_message of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_message() takes exactly 3 arguments (2 given)
[ERROR:websocket] 2018-08-20 07:26:55,253 - error from callback <bound method CertStreamClient._on_message of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_message() takes exactly 3 arguments (2 given)
[ERROR:websocket] 2018-08-20 07:26:55,484 - error from callback <bound method CertStreamClient._on_message of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_message() takes exactly 3 arguments (2 given)
[ERROR:websocket] 2018-08-20 07:26:55,496 - error from callback <bound method CertStreamClient._on_message of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_message() takes exactly 3 arguments (2 given)
[ERROR:websocket] 2018-08-20 07:26:55,509 - error from callback <bound method CertStreamClient._on_message of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_message() takes exactly 3 arguments (2 given)
[ERROR:websocket] 2018-08-20 07:26:56,093 - error from callback <bound method CertStreamClient._on_error of <certstream.core.CertStreamClient object at 0x7fb85883dc10>>: _on_error() takes exactly 3 arguments (2 given)

I'm not sure if this is caused by the certstream library using some deprecated functionality that's been removed or what's going on?

@fitzterra
Copy link

I'm seeing the same thing going from 0.50.0 to 0.51.0:

error from callback <bound method ConWebsock.on_error of <ConWebsock(Thread-1, started daemon 140390355556096)>>: on_error() takes exactly 3 arguments (2 given)

This is from mpfshell. Reverting back to 0.50.0 fixes the issue.
Changing line 332 in _app.py back to:

if inspect.ismethod(callback) and isinstance(callback.__self__, WebSocketApp)

as it was in 0.50.0 seems to fix the issue.

I'm not familiar enough with this code to understand what the isinstance check did before and why it was removed. It seems that with the test in place, the if condition fails, which executes the callback(self, *args) in the else leg, causing the missing args to the callbacks to now be included as expected by the client using this lib.

@nlevitt
Copy link
Contributor

nlevitt commented Aug 21, 2018

@liris These folks are correct, somehow the revert from #463 missed the thing that actually needed to be reverted, which is this:
https://github.com/websocket-client/websocket-client/pull/442/files#diff-afdc8659472527a33c2249875888fd1aL317

It did revert my pull request #462 though which means 0.51.0 is equally as broken as 0.49.0 :(

@gnu-user
Copy link

@liris Please fix this, it's affected myself as well as other colleagues I know that are using this library. The suggestion from @nlevitt should solve the problem.

@friday
Copy link

friday commented Sep 6, 2018

Seems like this was fixed in 0.52.0 a few days ago? At least I'm not getting the issue any more.

@swedishmike
Copy link
Author

As @friday says - 0.52.0 sorted this. Closing this one now.

@bor8
Copy link

bor8 commented Mar 8, 2019

I have the same problem in version 0.55.0. My workaround:

pip uninstall websocket
pip uninstall websocket-client
pip install websocket-client==0.52.0

@dnome2
Copy link

dnome2 commented Aug 8, 2019

0.56.0 still has the issue. any clue anyone here? @liris

@Gonzalliz
Copy link

0.57.0 still possess the error

@cjds
Copy link
Contributor

cjds commented Mar 16, 2020

The problem is here
https://github.com/websocket-client/websocket-client/blob/master/websocket/_app.py#L343 as has been stated above, which causes the error

 _on_message() takes exactly 3 arguments (2 given)

because WebSocket object is not sent to the callback method.

which is a use case for @MRSharff
#471

One way to work around this is to force your function to be a lambda instead of a function so that it misses the isMethod check from inspect

self._websocket = websocket.WebSocketApp(self._url ,
                                                 on_message=lambda ws, msg: self.on_message(ws, msg),
                                                 on_error=lambda ws, err: self.on_error(ws, err),
                                                 on_close=lambda ws: self.on_close(ws),
                                                 on_ping=lambda ws: self.on_ping(ws),
                                                 on_open=lambda ws: self.on_open(ws))

But I don't think this is the intention. @MRSharff is it possible to change your use case to be

class SomeObject:
    label='this is some object'
    print_label(self, ws):
        print(self.label)

someobj = SomeObject()
ws = websocket.WebsocketApp('ws://echo.websocket.org/', on_open=someobj.print_label)

which could be done and allow us to simplify the function here

https://github.com/websocket-client/websocket-client/blob/master/websocket/_app.py#L343

@cjds
Copy link
Contributor

cjds commented Mar 16, 2020

@liris can you comment on desired use of library as well?

@MRSharff
Copy link

MRSharff commented Apr 8, 2020

@cjds I believe I was incorrect in my original understanding of the code when I wrote my comments on #471 and now I'm not sure I have enough insight to comment.

Though, this quick little mockup seems to work fine:

class WebsocketApp:
    def __init__(self, on_open=None):
        self.on_open = on_open

    def _callback(self, callback, *args):
        if callback:
            callback(self, *args)

    def run(self):
        self._callback(self.on_open)

class SomeObject:
    label = 'some object'
    def print_label(self, ws):
        print(self.label)

def main():
    someobj = SomeObject()
    ws = WebsocketApp(on_open=someobj.print_label)
    ws.run()

if __name__ == '__main__':
    main()
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants