658395 - Running an endurance test with restarts enabled and a large number of iterations causes Disconnect Error

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Steps to reproduce:

Run the following from within mozmill-automation:
./testrun_endurance.py --iterations=1000 /Applications/Firefox.app/

After the first test finishes the browser is restarted and then closes. The following is output to the console:

TEST-START | /var/folders/YW/YW0PWstZHSurB3MGeMrD+E+++TI/-Tmp-/tmpdmjR_f.mozmill-tests/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js | setupModule
TEST-PASS | /var/folders/YW/YW0PWstZHSurB3MGeMrD+E+++TI/-Tmp-/tmpdmjR_f.mozmill-tests/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js | setupModule
TEST-START | /var/folders/YW/YW0PWstZHSurB3MGeMrD+E+++TI/-Tmp-/tmpdmjR_f.mozmill-tests/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js | testOpenAndCloseAddonManager
Running global cleanup code from study base classes.
TEST-PASS | /var/folders/YW/YW0PWstZHSurB3MGeMrD+E+++TI/-Tmp-/tmpdmjR_f.mozmill-tests/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js | testOpenAndCloseAddonManager
TEST-START | /var/folders/YW/YW0PWstZHSurB3MGeMrD+E+++TI/-Tmp-/tmpdmjR_f.mozmill-tests/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js | teardownModule
TEST-PASS | /var/folders/YW/YW0PWstZHSurB3MGeMrD+E+++TI/-Tmp-/tmpdmjR_f.mozmill-tests/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js | teardownModule
TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed
INFO Passed: 3
INFO Failed: 1
INFO Skipped: 0

Comment 1

[cmtalbert]

You really need to instrument the code around: https://github.com/mozautomation/mozmill/blob/hotfix-1.5/jsbridge/jsbridge/extension/resource/modules/server.js#L244  to get an idea if this is the old "too much data" error.

Ideally we'd want to know what the string.length is, and we'd want to turn on all the logging that shows how much data we're sending over in each chunk. Actually logging the string itself is less important and probably just would make the data noisy.

Comment 2

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

(In reply to comment #1)
> You really need to instrument the code around:

Would be good if you could setup some documentation for Mozmill how to do that. While you are doing it probably more often, we nearly never have to do that. Thanks.

Comment 3

[cmtalbert]

(In reply to comment #2)
> (In reply to comment #1)
> > You really need to instrument the code around:
> 
> Would be good if you could setup some documentation for Mozmill how to do
> that. While you are doing it probably more often, we nearly never have to do
> that. Thanks.

You uncomment the log statement in server.js and just add log(string.length) in various places.  Do you need full documentation on that?  I linked to the code.  We can talk about it online if needed.

Comment 4

[cmtalbert]

Dave,
I've got an even easier task for you.  Would it be possible to run the endurance test to produce the last persisted object before the failure occurs?  I think this should be pretty easy if you bisect on the number of iterations.  If you can attach that persisted object to this bug (or just tell me how big it is) that will probably give us the smoking gun we need to get started debugging.

Also, I'm a bit unsure of steps to repro, please correct where I'm wrong:
1. Checkout mozmill-tests (default branch)
2. Checkout mozmill-automation (any branch?)
3. Run with command line in comment 0?

Are you running mozmill 2.0 or mozmill 1.5.x?

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've just tried replicating this using mozmill-restart directly and the tests run all the way through, so I suspect there's something in our automation project that's causing the issue.

Clint: I'm using Mozmil 1.5.x, and your steps to reproduce are correct, except that you don't need to checkout the mozmill-tests project.

Comment 6

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

Dave, can you please update testrun.py to not use the root folder of endurance tests but the one of the addons manager test? I wonder if it also happens if you only run this folder.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This doesn't replicate the problem. That folder only has one test in it, and the issue occurs after the browser has restarted. On my machine, 200 iterations causes the issue after the first test. 100 iterations causes the issue after the second test.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Enable endurance test restarts. v1.0

This patch will enable endurance tests restarts.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Steps to reproduce:
1. Checkout mozmill-automation (http://hg.mozilla.org/qa/mozmill-automation)
2. Apply patch in attachment 536990 [details] [diff] [review] to enable restarts between tests
3. Run ./testrun_endurance.py --iterations=100 /path/to/Firefox

Comment 10

[cmtalbert]

Attachment: A play by play diagnosis of the issue

So, this patch demonstrates the problem.  We are indeed sending out > 64K of data and then upon trying to read that data back into JS the underlying networking infrastructure is truncating us to 64K.  Because we're truncated we don't have a syntactically sound JS expression, and when we eval that expression, it throws, the bridge disconnects, and puppies cry.

The most lightweight solution is to figure out whether we actually *need* that data back in JS.  I don't think your code really does anything with that data in JS, does it?  It just spools data into the persisted object and ships that data back to python.  I've verified that all the tests pass if I comment out the two places we send that data back into Javascript. If that is the case then I would propose we fix this the following way:

1. We allow you to specify whether the persisted data should be reflected back into JS.  We could put something on the frame that you could call which would determine whether or not the persisted data should come back into JS.  The default would be set so that data is reflected (current behavior).

2. We change the persisted object to allow for a spooling behavior (off by default) so that when set it concatenates new data to existing, rather than replace the existing value with the new one.  

3. And we change the two places in momzill/__init__.py where we reflect the persisted object back into JS so that it only does that conditionally based on the settings above.

What do y'all think?

Comment 11

[Jeff Hammel]

I don't really understand the architecture, etc, of the endurance tests so I'm not really sure if I can give a good assessment (though I'd be happy to talk in #mozmill if someone can point me to/explain the reasons for the code).

I *do* think we should (if we can) detect if an object is too big, jsbridge-side (well, technically, *both* sides of the bridge), and send a more sensible error.

I'm more hesitent to overload persisted to do other than what it does now.  Its pretty straight-forward as to what its for.

I'm not sure why persisted is being used here.  persisted is for objects that should be shared by JS and python. Or from a more pragmatic point of view, persisted is for objects that need to cross the bridge more than once (that is, at least python -> JS -> back).  It doesn't sound like this is what its being used for here? 

Does the data come from python-side? JS side? both? mozmill has an excellent event system.  While the API is more blatantly exposed in trunk (2.0), with 1.5x it is still possible to subclass and add what events you want.  

Anyway, I'm more than happy to help find a solution here if someone can precisely describe the problem and point me to code.  I don't really want to try to intuit this from mozmill-automation, as this could be (very) time-consuming, but if someone can point me to the bits I need to know, I'd be happy to help.  I'd rather try to figure out if there is a better way of solving this problem.

I am in favor of :ctalbert's suggestion 2., though I would be more inclined to turn on spooling once/if it is needed vs having the user have to know if they are breaking the (fairly arbitrary and possibly system-dependent) socket-size limit.  We should probably have spooling anyway, not just for persisted, but since mozmill/jsbridge are being used more like industrial-strength tools, they should behave like industrial-strength tools.  Whether this is advisable for 1.5x, I'm not really sure.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I think our discussion in the MM meeting today was effectively option 4:

4) Build a custom event listener/callback in Python to handle the endurance test data stream. Call it from the JS module used for endurance instead of adding to persisted object. Custom event listener persists to disk instead of persisted object.

I agreed that this is likely the least invasive and easiest option, and Clint mentioned working up a quick proof of concept for us to take a look at.

Comment 13

[Jeff Hammel]

So surveying endurance.js and testrun.py persisted is being used for something other than what its really intended.  persisted objects are share between python and javascript. For instance, addons is sent from JS back to the report for a single result: http://hg.mozilla.org/qa/mozmill-tests/file/e260c600a7c3/lib/endurance.js#l66 . However, we transfer this everytime.

Is there a single valid instance of using persisted in these tests?

My inclination is to take out all use of persisted and replace them by selective fetching and events.

Comment 14

[Jeff Hammel]

Anyone have a clue why this lives on this?

http://hg.mozilla.org/qa/mozmill-tests/file/e260c600a7c3/lib/endurance.js#l90

Comment 15

[Jeff Hammel]

Attachment: modified endurance.js

Comment 16

[Jeff Hammel]

Attachment: illustrate use of event system

This uses events to get information from JS -> python.  ABICT, persisted shouldn't be used at *all* for any of the things its used for e.g. addons.  JSObject should be used to introspect/modify data from python -> JS, and events should be used to get information from JS -> python, as I've done here.  Using persisted to collect volumes of information *will* cause this to happen again!  I am surprised addons works.  It won't for a moderate number of addons. I'm more than happy to help getting things to work in a robust way, but have noticed that they're only problems once they've already become emergencies.  I'd like to start preempting that.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update automation script to use event listeners. v1.0

This patch changes the installed addons and endurance results lists to use event listeners instead of the persisted object.

Also included is an optimisation of the populate_metrics method (thanks Jeff!), and changing the defaults so that tests run in restart mode.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update endurance shared module to fire events. v1.0

Updates the shared module to fire events instead of writing to the persisted object.

Results generated without the patches applied:
http://mozmill.blargon7.com/#/endurance/report/88b31ff45aa5a2c9f30fa1462d034b92

Results generated with the patches applied:
http://mozmill.blargon7.com/#/endurance/report/88b31ff45aa5a2c9f30fa1462d0359ce

I have also run a test with 2000 iterations with the patches applied and it completed without a disconnect error. Unfortunately there seem to be issues with displaying the results for this test run, perhaps due to its size. The report can be seen here, although it may take a while to load: http://mozmill.blargon7.com/#/endurance/report/88b31ff45aa5a2c9f30fa1462d03632e

Comment 19

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

(In reply to comment #16)
> JSObject should be used to introspect/modify data from python -> JS, and
> events should be used to get information from JS -> python, as I've done

Jeff, do you have an example how to retrieve data via JSObject from JS side? For example take the software update tests, where test2 depends on collected information from test1 which is stored in the persisted object. Does a situation like that still qualify to use persisted? If not we will have to change the whole system here too but that should happen on its own bug.

Comment 20

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

Dave, with the changes from above it should be easy enough now to cache data on disk.

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #19)
> (In reply to comment #16)
> > JSObject should be used to introspect/modify data from python -> JS, and
> > events should be used to get information from JS -> python, as I've done
> 
> Jeff, do you have an example how to retrieve data via JSObject from JS side?
> For example take the software update tests, where test2 depends on collected
> information from test1 which is stored in the persisted object. Does a
> situation like that still qualify to use persisted? If not we will have to
> change the whole system here too but that should happen on its own bug.

Yeah, second this. Generally, I'd like to not have to use a more complicated system if persisted is acceptable, so knowing the cases where it's ok to use would be good. My understanding is that we're ok up to 64kb of data, so very small stuff like flags, etc. should never be an issue, right?

Comment 22

[Jeff Hammel]

(In reply to comment #19)
> (In reply to comment #16)
> > JSObject should be used to introspect/modify data from python -> JS, and
> > events should be used to get information from JS -> python, as I've done
> 
> Jeff, do you have an example how to retrieve data via JSObject from JS side?
> For example take the software update tests, where test2 depends on collected
> information from test1 which is stored in the persisted object. Does a
> situation like that still qualify to use persisted? If not we will have to
> change the whole system here too but that should happen on its own bug.

Not sure if I understand the question. If you're going from JS -> python, which is how I read this, see my attachments for how this can be done via event listeners (which is probably always the correct way to do JS -> python communication for the forseeable future). If the question is how do python -> JS communication via JSObject, see https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L387

Comment 23

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

(In reply to comment #22)
> communication for the forseeable future). If the question is how do python
> -> JS communication via JSObject, see
> https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/
> __init__.py#L387

This is still python code. I need a way in a Mozmill test itself to access data which has been transferred via an event listener before.

Comment 24

[Jeff Hammel]

Comment on attachment 540465 [details] [diff] [review]
Update automation script to use event listeners. v1.0

     parser.add_option("--no-restart",
                       action="store_false",
                       dest="restart_tests",
-                      default=False,
+                      default=True,
Not sure why this changed, but other than that looks good

Comment 25

[Jeff Hammel]

Comment on attachment 540468 [details] [diff] [review]
Update endurance shared module to fire events. v1.0

Looks good.  In general, things to notice is that now every test that you care about having addons data in will need to fire the event, as done here.  I'm not which these are.  In general, mozmill-automation and mozmill-tests are coupled which leads to interesting dependency problems.  Also noting that in 2.0 addons will be fetched as part of get_appinfo so you'll no longer need to fetch them yourself at that point

Comment 26

[Jeff Hammel]

(In reply to comment #23)
> (In reply to comment #22)
> > communication for the forseeable future). If the question is how do python
> > -> JS communication via JSObject, see
> > https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/
> > __init__.py#L387
> 
> This is still python code. I need a way in a Mozmill test itself to access
> data which has been transferred via an event listener before.

Still not sure I understand.  Do you want to know how to transfer data from python to JS? e.g.

mozmill = jsbridge.JSObject(bridge, mozmillModuleJs)
mozmill.foo = 'bar' # etc

Comment 27

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

When we have situations with a lot of data involved and we have to pass this data from one restart test to the next, how can this be done? I.e. if we need some of the information we will collect in the future for endurance tests in the last test of a single restart folder, what has to be done? The persisted object is completely managed by Mozmill but the events path not.

Comment 28

[Jeff Hammel]

I don't think this bug is really the right venue for discussion about general how to communicate between JS and python in mozmill. Again, to be brief, you can transfer data to and from JS using JSObject or you can use events.  We can also provide a front end to JSObject if there is a need for a convenience interface. persisted is really only for sharing data between the too. Using persisted as a convenience cache will cause excess transfers and latency and has the danger of overloading the 64k limit. I'm happy to talk about this all more at length on mozmill-dev or an appropriate forum but IMHO this is way beyond the scope of the bug

Comment 29

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

Comment on attachment 540468 [details] [diff] [review]
Update endurance shared module to fire events. v1.0

Looks good to me.

Comment 30

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

Comment on attachment 540465 [details] [diff] [review]
Update automation script to use event listeners. v1.0

>         if self.timeout:
>-          self._mozmill.jsbridge_timeout = self.timeout
>+             self._mozmill.jsbridge_timeout = self.timeout

The indentation is still off by one blank. 

>-        self._mozmill.persisted['addons'] = [ ]
>+        self.installed_addons = []
>+        self._mozmill.mozmill.add_listener(self.addons_event, eventType='mozmill.installedAddons')

The registration of the listener has to happen only once and should take place in the init method. I would also duplicate the initialization of self.installed_addons to the init method. Otherwise it's hard to see that such a property exists.


>-            if key in dict:
>-                if isinstance(data[key], types.ListType):
>-                    # data is already a list so extend
>-                    dict[key].extend(data[key])
>-                else:
>-                    # data is not a list so append
>-                    dict[key].append(data[key])
>-            else:
>-                # does not contain the key so we create a new list
>-                if isinstance(data[key], types.ListType):
>-                    # data is already a list
>-                    dict[key] = data[key]
>-                else:
>-                    # data is not a list so create a list
>-                    dict[key] = [data[key]]
>+            _data = data[key]
>+            if not isinstance(_data, list):
>+                _data = [_data]
>+            dict.setdefault(key, []).extend(_data)

Wow, slick code.

>     def prepare_tests(self):
>         TestRun.prepare_tests(self)
>+        self._mozmill.mozmill.add_listener(self.endurance_event, eventType='mozmill.enduranceResults')

As mentioned above, please do that in the init method.

>     def update_report(self, report):
>+        # get basic report
>         TestRun.update_report(self, report)
> 
>-        report['endurance'] = self._mozmill.persisted['endurance']
>+        # update report with endurance data
>+        report['endurance'] = {'delay': self.delay,
>+                               'iterations': self.iterations,
>+                               'restart': self.restart_tests,
>+                               'results': self.endurance_results}

Can't you simply extend self._mozmill.persisted['endurance'] with the results?

Comment 31

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

Thanks Jeff, I will send out an email to mozmill-dev in a couple of minutes.

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update automation script to use event listeners. v1.1

(In reply to comment #30)
> Comment on attachment 540465 [details] [diff] [review] [review]
> Update automation script to use event listeners. v1.0
> 
> >         if self.timeout:
> >-          self._mozmill.jsbridge_timeout = self.timeout
> >+             self._mozmill.jsbridge_timeout = self.timeout
> 
> The indentation is still off by one blank.

Well spotted. Fixed.

> >-        self._mozmill.persisted['addons'] = [ ]
> >+        self.installed_addons = []
> >+        self._mozmill.mozmill.add_listener(self.addons_event, eventType='mozmill.installedAddons')
> 
> The registration of the listener has to happen only once and should take
> place in the init method. I would also duplicate the initialization of
> self.installed_addons to the init method. Otherwise it's hard to see that
> such a property exists.

As discussed on IRC, it's not possible to move the registration of the listener to init as self._mozmill is not defined at that time.

Also, it was decided to leave the initialisation of the installed addons list where it is so that it's cleared before each testrun.

> >-            if key in dict:
> >-                if isinstance(data[key], types.ListType):
> >-                    # data is already a list so extend
> >-                    dict[key].extend(data[key])
> >-                else:
> >-                    # data is not a list so append
> >-                    dict[key].append(data[key])
> >-            else:
> >-                # does not contain the key so we create a new list
> >-                if isinstance(data[key], types.ListType):
> >-                    # data is already a list
> >-                    dict[key] = data[key]
> >-                else:
> >-                    # data is not a list so create a list
> >-                    dict[key] = [data[key]]
> >+            _data = data[key]
> >+            if not isinstance(_data, list):
> >+                _data = [_data]
> >+            dict.setdefault(key, []).extend(_data)
> 
> Wow, slick code.

I know, but I can't take credit - it's all Jeff's work.

> >     def prepare_tests(self):
> >         TestRun.prepare_tests(self)
> >+        self._mozmill.mozmill.add_listener(self.endurance_event, eventType='mozmill.enduranceResults')
> 
> As mentioned above, please do that in the init method.

Left where it is for the above mentioned reason.

> >     def update_report(self, report):
> >+        # get basic report
> >         TestRun.update_report(self, report)
> > 
> >-        report['endurance'] = self._mozmill.persisted['endurance']
> >+        # update report with endurance data
> >+        report['endurance'] = {'delay': self.delay,
> >+                               'iterations': self.iterations,
> >+                               'restart': self.restart_tests,
> >+                               'results': self.endurance_results}
> 
> Can't you simply extend self._mozmill.persisted['endurance'] with the
> results?

As far as I can tell I can't extend a dictionary. I have improved this, but let me know if this is not what you meant.

Comment 33

[Jeff Hammel]

If you extend (.update, really) the persisted dict you will have the same problem as before: it will be transferred over the wire and the rest of the work will be for nought, if i understand the suggestion correctly

Comment 34

[Jeff Hammel]

So I don't know if this is a mozmill bug, per se.  If you want to ticket the 64k limit over jsbridge separately we can keep track of it, but I would highly doubt for 2.0. It will be a big deal

Comment 35

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

Comment on attachment 544048 [details] [diff] [review]
Update automation script to use event listeners. v1.1

>+    def addons_event(self, obj):
>+        self.installed_addons = obj
>+

I did a closer look again on this patch and everything looks fine except this part. This listener will be called each time the Endurance constructor is called. Means for each test which gets executed. Whenever we have an endurance test which installs additional add-ons the list is not accurate anymore. We should only do the assignment once for the very first test and never again for the current test-run.

Comment 36

[Dave Hunt [:davehunt] [he/him] ⌚BST]

How would we fire this listener only on the first test in a testrun? We don't have any endurance tests that install additional addons, and if we did why wouldn't we want this to be reflected in the installed addons list?

Comment 37

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

The listener will get fired all the time but it's something we can check on the Python side. Only assign the list of add-ons once.

Comment 38

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update automation script to use event listeners. v1.2

(In reply to comment #37)
> The listener will get fired all the time but it's something we can check on
> the Python side. Only assign the list of add-ons once.

Done.

Comment 39

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

Comment on attachment 545351 [details] [diff] [review]
Update automation script to use event listeners. v1.2

>+    def addons_event(self, obj):
>+        if not self.installed_addons:
>+            self.installed_addons = obj

Even the situation I will mention now will not happen, we should keep the logic in-tact. The case above will fail if there would be no add-ons installed (obj will be None). Once a test installs an add-on and Firefox restarts, we will send one item in obj. Because the installed_addons list is empty, we will overwrite it. Now we have incorrect data in the array. While it has to be None, an add-on is listed now.

To fix that we should have a flag set, which really exits the method if it has already been run once. Otherwise we should simply unregister the listener, which would be better code.

Comment 40

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update automation script to use event listeners. v1.3

(In reply to comment #39)
> Comment on attachment 545351 [details] [diff] [review] [review]
> Update automation script to use event listeners. v1.2
> 
> >+    def addons_event(self, obj):
> >+        if not self.installed_addons:
> >+            self.installed_addons = obj
> 
> Even the situation I will mention now will not happen, we should keep the
> logic in-tact. The case above will fail if there would be no add-ons
> installed (obj will be None). Once a test installs an add-on and Firefox
> restarts, we will send one item in obj. Because the installed_addons list is
> empty, we will overwrite it. Now we have incorrect data in the array. While
> it has to be None, an add-on is listed now.
> 
> To fix that we should have a flag set, which really exits the method if it
> has already been run once.

Done.

> Otherwise we should simply unregister the
> listener, which would be better code.

Jeff: Is there a way to remove/unregister a listener as Henrik suggests? I couldn't see a way to do this.

Comment 41

[Jeff Hammel]

Comment on attachment 546144 [details] [diff] [review]
Update automation script to use event listeners. v1.3

wfm

Comment 42

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

Jeff, do you also have any feedback for the removeListener question Dave asked for? Thanks.

Comment 43

[Jeff Hammel]

(In reply to comment #42)
> Jeff, do you also have any feedback for the removeListener question Dave
> asked for? Thanks.

There is no API for this currently.  Feel free to file a bug, although its probably low priority IMHO since listeners are free to ignore events sent to them.

Comment 44

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

(In reply to comment #43)
> There is no API for this currently.  Feel free to file a bug, although its
> probably low priority IMHO since listeners are free to ignore events sent to
> them.

This sounds bad and seems to be connected to data loss. It's not something we could really base our code on for critical data. The persisted is still the main path of transferring data. Dave, please file two bugs so we can cover both issues.

Comment 45

[Jeff Hammel]

I'm not sure what you mean by data loss here.  You can also go through and manually unregister an event.  You can possibly simplify this with decorators too, although I'm not sure I understand how this should look in practice.  Can someone supply pseudo-code for what is desired (i.e. how it would be called in a program), possibly on the follow-up bug?

Comment 46

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

Comment on attachment 546144 [details] [diff] [review]
Update automation script to use event listeners. v1.3

Given the current limitation of removeListener lets get this code in so that we can run our endurance tests as restart tests.

Comment 47

[cmtalbert]

I think there is some confusion here around what problem we're trying to solve with regard to unregistering the listeners.  I think what Henrik wants (Henrik, please correct if I'm wrong) is a way to unregister the listener so that his code can be certain it is only called once, in other words it goes through this process:
* register listener for knowing if addons are installed
* get event saying addons are installed
* add installed addons to array
* restart browser
* get another event saying no new addons installed
* have to realize you've already been called and should do something different now with regard to the addon array.

I think what Henrik wants instead of having a flag to determine that the addons array has already been modified is he wants to do the following:
* register listener for knowing if addons are installed
* get event saying addons are installed
* add installed addons to array
* '''unregister event listener'''
* restart browser
* don't get anymore events.

The problem with this style of coding is that you *must* be certain that you will never need those listeners again.  Because once you unregister the listener, you cannot re-register the listener without missing events.  The issue here is that once you unregister the listener, you will be in a race condition with the browser and you will almost certainly miss events if you want to re-register it.

So, I'd really recommend the flag architecture that you guys originally decided on for this specific use case.  In general, there should be a "removelistener" API for any "addlistener" API, that's just good design.  So that should be created, but I think using "removelistener" for this use case is going to open a can of worms that will bite you the next time you try to extend this code.

Comment 48

[Jeff Hammel]

One way to ensure a function is only called once is a decorator, e.g.:

class fireonce(object):
    def __init__(self, func):
        self.func = func
    def __call__(self, *args, **kwargs):
        if not self.func:
            return None
        retval = self.func(*args, **kwargs)
        self.func = None

@fireonce
def foo(x):
    print x

foo('bar')
foo('fleem') # not printed

Comment 49

[Jeff Hammel]

Oh, and i forgot to `return retval`. as said, example code ;) though since event listeners return none it actually doesn't matter here

Comment 50

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

(In reply to comment #47)
> I think what Henrik wants instead of having a flag to determine that the
> addons array has already been modified is he wants to do the following:
> * register listener for knowing if addons are installed
> * get event saying addons are installed
> * add installed addons to array
> * '''unregister event listener'''
> * restart browser
> * don't get anymore events.
> 
> The problem with this style of coding is that you *must* be certain that you
> will never need those listeners again.  Because once you unregister the
> listener, you cannot re-register the listener without missing events.  The
> issue here is that once you unregister the listener, you will be in a race
> condition with the browser and you will almost certainly miss events if you
> want to re-register it.

That's exactly what we want to have here. We are aware of the situation that events sent while no listener is attached will be lost. But that raises another question to Jeff. Is that what you were talking about ignoring events in comment 43?

Comment 51

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update endurance shared module to fire events. v1.1

Updated to apply cleanly.

Comment 52

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as (mozmill-tests):
http://hg.mozilla.org/qa/mozmill-tests/rev/f9e62328055f (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/18e771cdbf85 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/54d0f03a155c (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/b3e1e2af5722 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/86f61fd8e602 (mozilla-2.0)

Landed as (mozmill-automation):
http://hg.mozilla.org/qa/mozmill-automation/rev/b53d85e807da