656704 - read_callback is defined but never used

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

https://github.com/mozautomation/mozmill/blob/master/jsbridge/jsbridge/network.py#L95

This is not refered to anywhere inside the mozmill codebase.  Nor is
it part of asyncore.dispatcher or sockets.  It should be removed ABICT.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The updated location is at:
https://github.com/mozilla/mozmill/blob/master/jsbridge/jsbridge/network.py#L67

Comment 2

[piotr.repetowski]

Hi, I would like to work on this issue. I cloned git repository and found read_callback declaration - I can remove that line. What is the next step?

Best regards

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Piotr! For the next steps please have a look here:
https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Working_on_Mozbase_and_Contributing_Patches

You will have to attach your patch to the bug and ask for review from Jeff or myself.

Comment 4

[Jeff Hammel]

This is a mozmill bug, not a mozbase bug, so https://wiki.mozilla.org/Auto-tools/Projects/Mozmill is probably the place to look

Comment 5

[piotr.repetowski]

Attachment: Remove read_callback patch

Comment 6

[piotr.repetowski]

Thank you for the information. I have attached a patch. Henrik, can you review my changes?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 681250 [details] [diff] [review]
Remove read_callback patch

Review of attachment 681250 [details] [diff] [review]:
-----------------------------------------------------------------

::: jsbridge/jsbridge/network.py
@@ -62,4 @@
>          self.data = self.read_all()
>          self.process_read(self.data)
>  
> -    read_callback = lambda self, data: None

You should not remove data because that can break handle_read(). But if we do it, we probably only have to update the above mentioned method.

Jeff, can I get your feedback on?

Comment 8

[Jeff Hammel]

Comment on attachment 681250 [details] [diff] [review]
Remove read_callback patch

Damned if I know.  If it is needed and/or part of the telnet API is overriding should probably be documented.  Also, it should probably be an actual function instead of a lambda.  I have no idea, in fact, if it is needed or what the behaviour of deleting is.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I haven't found any instance of data in asyncore.dispatcher. So I assume it a property we create on our Telnet class.

Comment 10

[piotr.repetowski]

Henrik I searched the mozmill and the asyncore.dispatcher code and I did not find the use of the read_callback.

Can you give me some tips what to do next? I want to help but at the moment my knowledge is not enough.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We do not question the read_callback but the data property followed after the colon in the same line:

> -    read_callback = lambda self, data: None

You are removing that too but it's been used in handle_read(). It's getting written there before it's read out but for safety we should be really sure before removing `data`. If we do that we should most likely rename self.data in handle_read() to `data`.

Comment 12

[piotr.repetowski]

Attachment: Remove read_callback, make handle_read safer patch

Thanks for the explanation. I have modified 'data' in handle_read(). Now 'data' is a local variable not a class member. You are right previous approach wasn't safe.

Henrik, correct me if I am wrong. In my first patch I removed 'data' - method argument. That shouldn't break handle_read().

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 682241 [details] [diff] [review]
Remove read_callback, make handle_read safer patch

Review of attachment 682241 [details] [diff] [review]:
-----------------------------------------------------------------

I think that looks fine now. Thank you for the patch! If you are interested in fixing some more please let me know or join us in the #automation or #ateam channel on IRC.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://github.com/mozilla/mozmill/commit/4a0f4e961895401a3ee2849cdbb8d369727e9d3b

Comment 15

[Brian Marshall]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> We do not question the read_callback but the data property followed after
> the colon in the same line:
> 
> > -    read_callback = lambda self, data: None
> 
> You are removing that too but it's been used in handle_read(). It's getting
> written there before it's read out but for safety we should be really sure
> before removing `data`. If we do that we should most likely rename self.data
> in handle_read() to `data`.

Sorry, I know this bug is closed, but I don't think there's a data property defined on that line. data is an argument to the lambda function (as piotr mentioned in comment 12) - i.e. a local variable instead of a class property. It should be equivalent to:

    read_callback = lambda (self, data): None
or
    def read_callback(self, data): return None