read_callback is defined but never used

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Jeff Hammel, Assigned: piotr.repetowski)

Tracking

Details

(Whiteboard: [mentor=whimboo][lang=py][good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
The updated location is at:
https://github.com/mozilla/mozmill/blob/master/jsbridge/jsbridge/network.py#L67
Whiteboard: [mentor=whimboo][lang=py][good first bug]
(Assignee)

Comment 2

5 years ago
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
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.
Assignee: nobody → piotr.repetowski
Status: NEW → ASSIGNED
(Reporter)

Comment 4

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

Comment 5

5 years ago
Created attachment 681250 [details] [diff] [review]
Remove read_callback patch
Attachment #681250 - Flags: review?(hskupin)
(Assignee)

Comment 6

5 years ago
Thank you for the information. I have attached a patch. Henrik, can you review my changes?
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?
Attachment #681250 - Flags: review?(hskupin)
Attachment #681250 - Flags: review-
Attachment #681250 - Flags: feedback?(jhammel)
(Reporter)

Comment 8

5 years ago
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.
Attachment #681250 - Flags: feedback?(jhammel)
I haven't found any instance of data in asyncore.dispatcher. So I assume it a property we create on our Telnet class.
(Assignee)

Comment 10

5 years ago
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.
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`.
(Assignee)

Comment 12

5 years ago
Created attachment 682241 [details] [diff] [review]
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().
Attachment #681250 - Attachment is obsolete: true
Attachment #682241 - Flags: review?(hskupin)
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.
Attachment #682241 - Flags: review?(hskupin) → review+
https://github.com/mozilla/mozmill/commit/4a0f4e961895401a3ee2849cdbb8d369727e9d3b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 15

5 years ago
(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
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.