819068 - [OS.File] Waiting until all pending operations are complete

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

It seems that some clients of OS.File could make use of a function that notifies once all pending operations are complete.

This should be rather easy to implement. In osfile_async.jsm, add a function OS.File.wait that:
- if the queue is empty, returns an already resolved promise;
- if the queue is not empty, returns a promise that will be resolved upon completion of the last promise in the queue.

Comment 1

[Eduard Neculaesi (:eduardn)]

I would like to work on this bug.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Have fun :)

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Any news?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

In the absence of news, de-assigning the bug. If anyone wants to work on it, it's free.

Comment 5

[Prasanth Balakrishnan]

Would like to work on this bug. I am new to Mozilla devt and hence would require big help to get it started. I couldn't find a file osfile_async.jsm but could find files like osfile.jsm, osfile_async_front.jsm and osfile_async_worker.js etc. Are you talking about any of these files? Which file should i add the method to?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Hi Prasanth and thanks for offering to work on this bug.

Indeed, osfile_async.jsm doesn't exist anymore, it has been split in several chunks.

The best way to do this is to patch:
- osfile_async_front.jsm (the public API) to offer the method;
- _PromiseWorker.jsm to actually define the implementation.

If you need help or more info, do not hesitate to ask here or over IRC. I will be a little hard to reach during the next few days, but I will do my best to reply to your questions.

Comment 7

[Prasanth Balakrishnan]

Thank you for assigning the bug to me :) I would start working on the same from today.

To start off, could you point me to a similar implementation in the same file or another which i can use as reference.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Take a look at method |post| in |_PromiseWorker|. The function you need to write is mostly a very simplified version of |post|.

Comment 9

[Prasanth Balakrishnan]

Attachment: First cut of the new function wait

Added wait method to _PromiseWorker.jsm and lastElement property to the Queue.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 737204 [details] [diff] [review]
First cut of the new function wait

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

Good start. Next step: I want tests :)

::: toolkit/components/osfile/_PromiseWorker.jsm
@@ +65,5 @@
>     * @type {Queue<{deferred:deferred, closure:*=}>}
>     */
>    this._queue = new Queue();
> +  Object.defineProperty(_queue, "lastElement", {get : function() { return this._array[this._array.length - 1]; },
> +												writeable : false });

You should rather add this to the prototype of Queue.
Also, please avoid compressing something in one line.

@@ +169,5 @@
> +
> +  /**
> +   * Notifies when all pending operations are complete.
> +   *
> +   * @return {promise}

Could you document the return value?

@@ +171,5 @@
> +   * Notifies when all pending operations are complete.
> +   *
> +   * @return {promise}
> +   */
> +   wait: function wait() {

Nit: Indentation doesn't match the rest of the file.

@@ +175,5 @@
> +   wait: function wait() {
> +	if(_queue.isEmpty) {
> +		return Promise.resolve();
> +	}
> +	else {

Nit: } else {

@@ +177,5 @@
> +		return Promise.resolve();
> +	}
> +	else {
> +		let deferred = Promise.defer();
> +		let resolve = function() { deferred.resolve() };

Nit: either
  function() {
    deferred.resolve();
  }
or
  function() deferred.resolve();

Comment 11

[Prasanth Balakrishnan]

Attachment: Patch version 2 with review comments implemented

New patch with the review comments implemented. Reading about automated tests. Will soon start working on it. Sorry for the delay!

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 739214 [details] [diff] [review]
Patch version 2 with review comments implemented

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

::: toolkit/components/osfile/_PromiseWorker.jsm
@@ +32,5 @@
>      return this._array.length == 0;
> +  },
> +  lastElement: function lastElement() {
> +	var lastElementIndex = this._array.length - 1;
> +	return this._array[lastElementIndex];

Nit: please match the tab length of the rest of the file, here and in the rest of your patch.
Also, please use |let| instead of |var|.

@@ +171,5 @@
> +
> +  /**
> +   * Notifies when all pending operations are complete.
> +   *
> +   * @return {promise} A resolved promose if there are no pending

Nit: promise, not promose.

@@ +172,5 @@
> +  /**
> +   * Notifies when all pending operations are complete.
> +   *
> +   * @return {promise} A resolved promose if there are no pending
> +   * operations or else a deferred promise that will be resolved

Nit: upon.

@@ +176,5 @@
> +   * operations or else a deferred promise that will be resolved
> +   * completion of the last operation.
> +   */
> +  wait: function wait() {
> +	if(_queue.isEmpty) {

Not just |_queue|, |this._queue|.
Not |isEmpty|, |isEmpty()|.
Also, please add a whitespace between |if| and |(|.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Any news, Prasanth?

Comment 14

[Prasanth Balakrishnan]

Hi David.... I am here. But haven't been able to start work on tests yet. Should be starting something today.

Comment 15

[Prasanth Balakrishnan]

Hi David... I will be off for maybe another week or two more... Just thought of informing you if you are wondering about the status of the bug!

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Hi, are you planning to resume work on this bug?

Comment 17

[David Teller [:Yoric] - still alive but not very active]

No word for two months. De-assigning this bug. If anyone wants to pick it, it's available.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Now that bug 913899 has landed, this work has become easier.
We just need to expose |Scheduler.latestPromise|.

Comment 19

[Amod [:AMoz]]

I would like to work on this bug. Shall I take the patch ahead or start afresh as per comment 18. A bit more information would be helpful. Thanks !

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Hello Amod. You should probably start afresh.

The file to modify is the following: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm

Essentially, you just need to add a getter |OS.File.wait| that returns |Scheduler.latestPromise|.

Comment 21

[Amod [:AMoz]]

Attachment: patchv1

I have uploaded the patch to initiate the bug from my side. Regarding generating getters, I referred https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Working_with_Objects#Defining_getters_and_setters and returned | Scheduler.latestPromise | which was specified on http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm#l1026

This patch might be far from the actual one. Apologies for that.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 812264 [details] [diff] [review]
patchv1

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

Actually, that's good, thanks for the patch :)

I start to believe that |queue| is a better name than |wait|, could you make that change?
Also, please:
- move all of this towards the end of the file;
- respect the general layout of the file (e.g. two-spaces for indentation, move to the next line after each {, etc.).

Next, we'll need to think about a meaningful test for the feature.

Comment 23

[Amod [:AMoz]]

Attachment: patchv2

I have modified the patch as per the suggestions mentioned. 
Also, What should I do to understand the entire code in the file conceptually so that it would be easy to write tests ?

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 812444 [details] [diff] [review]
patchv2

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

Please fix as indicated below and add a commit message.
Regarding the tests, I believe that the best thing we can do is to create a mochitest in which:
- we enqueue many operations (say calling OS.File.writeAtomic 100 times);
- we wait OS.File.queue;
- once the wait is complete, we check whether the latest file actually exists.

You can draw inspiration, for instance, on http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_osfile_closed.js

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +962,5 @@
>  OS.File.DirectoryIterator = DirectoryIterator;
> +
> +/**
> +* A getter |OS.File.queue| that returns |Scheduler.latestPromise|
> +*/

That's not useful documentation.
/**
 * The latest operation queued.
 *
 * You may use this promise to ensure that all operations have completed before proceeding.
 */

@@ +963,5 @@
> +
> +/**
> +* A getter |OS.File.queue| that returns |Scheduler.latestPromise|
> +*/
> +Object.defineProperties(OS.File.queue, {

Just |Object.defineProperty| (singular).

@@ +967,5 @@
> +Object.defineProperties(OS.File.queue, {
> +  get: function() { 
> +    return Scheduler.latestPromise; 
> +  } 
> +});

Nit: you have whitespaces at the end of several lines, could you remove them?

Comment 25

[Abhishek Potnis [:avp]]

thanks for the review. Now I am heading towards Santa Clara for Mozilla Summit 2013. I will try to find time there for the bug. Else I would resume after the summit.

Comment 26

[Amod [:AMoz]]

hey Yoric...Sorry for the confusion... Actually I was using abhishek's laptop and forgot to check who was logged-in in the bugzilla account..

Comment 27

[Amod [:AMoz]]

I repeat the comment:

thanks for the review. Now I am heading towards Santa Clara for Mozilla Summit 2013. I will try to find time there for the bug. Else I would resume after the summit.

Comment 28

[Amod [:AMoz]]

Attachment: patchv3

will proceed towards test now.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 815514 [details] [diff] [review]
patchv3

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

That looks good. Now, on to adding tests.
Also, please mark your old patches as obsolete.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +1019,5 @@
> + */
> +Object.defineProperty(OS.File.queue, {
> + get: function() {
> +   return Scheduler.latestPromise;
> + } 

Nit: please remove that whitespace. Also, in this file, we use two spaces for indentation, not one.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Greatwarrior, what's your status?

Comment 31

[Amod [:AMoz]]

Attachment: patchv4

I apologize for the delay. When I pulled the latest copy of mozilla-central there were lot of reject files, so I manually applied the changes on the updated file. Also regarding test, I have gone through https://developer.mozilla.org/en/docs/Mochitest and I have some issues regarding its implementation. I will ask on #introduction on IRC for help.
Thanks !

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Looks like I didn't see the review? flag.
In the future, Amod, could you specify the reviewer? Otherwise, there are good chances that nobody will notice.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 830665 [details] [diff] [review]
patchv4

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

This should work.
Now, could you add a test?

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +1116,5 @@
> + */
> +Object.defineProperty(OS.File.queue, {
> +  get: function() {
> +    return Scheduler.latestPromise;
> + } 

Please remove the whitespaces at the end of lines.

Comment 34

[Amod [:AMoz]]

Sorry Yoric, Since I am busy with my academics, I wont be able to find time before March 2, 2014. If you feel that, it is too long to wait, you are free to assign it to someone more deserving. 
Thank you. Looking forward to work with you again after I return.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

In that case, let's unassign for the moment.

Comment 36

[Kushagra Singh [:kushagra]]

hi yoric. i would like to take up this bug and continue the work on it. could you please help me out?

Comment 37

[David Teller [:Yoric] - still alive but not very active]

(In reply to Kushagra Singh [:kushagra] from comment #36)
> hi yoric. i would like to take up this bug and continue the work on it.
> could you please help me out?

Sure, what do you need?

Comment 38

[Kushagra Singh [:kushagra]]

hi, do i just need to add test cases to amod's patch or is there anything else to change?

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Actually, it might be easier to wait until bug 957123 has landed.
Scheduler.queue will replace Scheduler.latestPromise and you will need to add test cases.

Comment 40

[Kushagra Singh [:kushagra]]

i have added myself in the cc list.will monitor its progress and get back to you as soon as this current bug is ready on work on.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

The blocker has been resolved. You can now go ahead, kushara.

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Any news, kushagra?

Comment 43

[Kushagra Singh [:kushagra]]

hi david, sorry for the delay. got stuck in tests,will work on it over the weekend.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Ok. No need to "needinfo?" me if you're not expecting an answer.

Comment 45

[Kushagra Singh [:kushagra]]

hi yoric,a couple of issues:-

1) i have gone through the mochitest documentations and having some difficulties building the test. i inquired on IRC and someone suggested that i do it using xpcshell citing that mochitest in some cases is an overkill.

2) in comment 24 u said we wait OS.File.queue. i am using setTimeOut() for the wait but cant decide upon the duration of the wait.

3) how to interpret the value returned by the OS.File.queue function?

Comment 46

[David Teller [:Yoric] - still alive but not very active]

(In reply to Kushagra Singh [:kushagra] from comment #45)
> hi yoric,a couple of issues:-
> 
> 1) i have gone through the mochitest documentations and having some
> difficulties building the test. i inquired on IRC and someone suggested that
> i do it using xpcshell citing that mochitest in some cases is an overkill.

Yes, please use a xpcshell test. You can find many examples in toolkit/components/osfile/tests/xpcshell

> 2) in comment 24 u said we wait OS.File.queue. i am using setTimeOut() for
> the wait but cant decide upon the duration of the wait.

You don't need setTimeout. You already have a value Scheduler.queue that i exactly what you need to return. No need to do more.

> 3) how to interpret the value returned by the OS.File.queue function?

I expect the value to be a Promise, just like Scheduler.queue.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Any news?

Comment 48

[Kushagra Singh [:kushagra]]

hey yoric :) sorry could not work on the bug for the past couple of weeks. exams and gsoc prep kept me occupied. will start working on the assigned bugs from this weekend, expect some updates over the weekend.

Comment 49

[Kushagra Singh [:kushagra]]

whenever I add Amod's patch to my local m-c source and build firefox, it seems to break something. The new FF build gets stuck at the checking Add-ons step at the time of initialization. Removing the patch and rebuilding fixes the problem.
Could you please verify this issue ? 
P.S I am using Scheduler.queue instead of Scheduler.latestPromise

Comment 50

[David Teller [:Yoric] - still alive but not very active]

Oh, right, I hadn't noticed but Amod's use of Object.defineProperty is incorrect, so this certainly raises an exception at some point.

Now, in the interest of pedagogy, I'm not going to tell you how to fix it, but I'm going to point you to the definition of Object.defineProperty so that you can find out the fix for yourself :)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty

Comment 51

[Kushagra Singh [:kushagra]]

ha :) that's fair. i have given this problem a go. let me know how it is, so that i could then proceed to test.

Comment 52

[Kushagra Singh [:kushagra]]

Attachment: revised patch for patchv4

Comment 53

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8400746 [details] [diff] [review]
revised patch for patchv4

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

Are you sure it's the right patch? I don't see any difference from the previous version.

Comment 54

[Kushagra Singh [:kushagra]]

yes it is.
Object.defineProperty(OS.File.queue, {
has been replaced by :
Object.defineProperty(OS.File,queue, {

Sorry for the confusion.

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8400746 [details] [diff] [review]
revised patch for patchv4

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

My bad, I misread.
This looks good, thanks.

Now, could you:
- apply the trivial change below;
- fold your patch and the previous patch.

After that, we'll want to add tests, as a new file test_queue.js in subdirectory osfile/tests/xpcshell. I would like to test the following properties:
- initially, OS.File.queue is resolved;
- after an operation that succeeds, OS.File.queue is still resolved;
- after an operation that fails, OS.File.queue is still resolved.

You can find more information on xpcshell tests here: https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests
And you can take a look at the other tests in the same directory.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +1305,5 @@
>   * The latest operation queued.
>   *
>   * You may use this promise to ensure that all operations have completed before proceeding.
>   */
>  Object.defineProperty(OS.File,queue, {

Nit: could you add a space between |OS.File,| and |queue|?

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Weird, I can't seem to clear the needinfo.

Comment 57

[Kushagra Singh [:kushagra]]

Attachment: add_OS.File.queue_and_tests.patch

Added the test case. Hope they are somewhere near accurate. Plus, even my implementation of OS.File.queue was incorrect. The current patch fixes that as well.
Replaces:
Object.defineProperty(OS.File,queue, {

With:
Object.defineProperty(OS.File, "queue", {

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8402218 [details] [diff] [review]
add_OS.File.queue_and_tests.patch

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

It's on the good path.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +1303,5 @@
>  
> +/**
> + * The latest operation queued.
> + *
> + * You may use this promise to ensure that all operations have completed before proceeding.

Let's take the opportunity to change the documentation of this property. This is not the latest operation queued anymore (I changed the definition of |queue| in another patch), just a promise resolved once all operations currently queued have been completed.

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini
@@ +27,5 @@
>  [test_duration.js]
>  [test_compression.js]
>  [test_osfile_writeAtomic_backupTo_option.js]
>  [test_osfile_error.js]
> +[test_queue.js]   	

Nit: Please remove the whitespace at the end of the line.

::: toolkit/components/osfile/tests/xpcshell/test_queue.js
@@ +5,5 @@
> +function run_test() {
> +  run_next_test();
> +}
> +// Check if Scheduler.queue returned by OS.File.queue is resolved initially.
> +add_task(function check_init() {

The new syntax is |function*|, as these are not real functions but generators. This applies to all the functions you pass as argument to |add_task|.

@@ +6,5 @@
> +  run_next_test();
> +}
> +// Check if Scheduler.queue returned by OS.File.queue is resolved initially.
> +add_task(function check_init() {
> +	let promise;

Please use an indentation of two characters.

@@ +18,5 @@
> +		do_check_true(check==1);
> +	})
> +	
> +	do_print("Function resolved");
> +

check_init doesn't wait for promise to be either resolved or rejected. To wait until it has been resolved or rejected, you need to return the resulting promise.
Here, instead of
  promise.then( ... )
you could do
  yield promise.then(... )

However, if you just wish to check that OS.File.queue is resolved, it is sufficient to do
  yield OS.File.queue

@@ +39,5 @@
> +		do_check_true(check==1);
> +	})
> +	
> +	do_print("Function resolved");
> +

Again, just call OS.File.open then yield OS.File.queue

@@ +44,5 @@
> +});
> +
> +// Check if Scheduler.queue returned by OS.File.queue is resolved 
> +// after an operation fails.
> +add_task(function check_failure() {

Again.

Comment 59

[Kushagra Singh [:kushagra]]

Attachment: add_OS.File.queue_and_tests_V2.patch

Made the corrections you asked for. Hope this fulfils all your requirements.

Comment 60

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8402612 [details] [diff] [review]
add_OS.File.queue_and_tests_V2.patch

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

Looks good to me, thanks for the patch.

Please:
- fix the indentation;
- add a commit message along the lines of "Bug 819068: Implementing OS.File.queue;r=Yoric".

Then we'll push this to the test server.

::: toolkit/components/osfile/tests/xpcshell/test_queue.js
@@ +30,5 @@
> +    yield OS.File.open(OS.Path.join(".", "Bigfoot"));
> +  } catch (err) {
> +      yield OS.File.queue;
> +      do_print("Function resolved");
> +	 }

Good, except the indentation.

Comment 61

[Kushagra Singh [:kushagra]]

Attachment: add_OS.File.queue_and_tests_V3.patch

this should be it.

Comment 62

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8402746 [details] [diff] [review]
add_OS.File.queue_and_tests_V3.patch

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

Almost landable, but I'd like a minor improvement to the test first.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +1304,5 @@
> +// Returns a resolved promise when all the queued operation have been completed.
> +Object.defineProperty(OS.File, "queue", {
> +  get: function() {
> +    return Scheduler.queue;
> + }

Nit: one more whitespace before }

::: toolkit/components/osfile/tests/xpcshell/test_queue.js
@@ +23,5 @@
> +});
> +
> +// Check if Scheduler.queue returned by OS.File.queue is resolved
> +// after an operation fails.
> +add_task(function* check_failure() {

I just realized that we're actually not checking that a failure has happened. You want to be able to perform a call to |do_check_true| at the end of the function to ensure that an exception was thrown.

Comment 63

[Kushagra Singh [:kushagra]]

Attachment: add_OS.File.queue_and_tests_V4.patch

Made all the changes you requested. Lets see how it performs :)

Comment 64

[David Teller [:Yoric] - still alive but not very active]

Try: ttps://tbpl.mozilla.org/?tree=Try&rev=98c920e2569e

Comment 65

[Kushagra Singh [:kushagra]]

all tests passed.

Comment 66

[David Teller [:Yoric] - still alive but not very active]

In that case, let's ship it!

Thanks for your contribution, I know that you have started with your next bug already and I'm looking forward to reviewing it.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

patching file toolkit/components/osfile/modules/osfile_async_front.jsm
bad hunk #1 @@ -1296,16 +1296,21 @@ this.OS.Shared = {
 (16 16 61 21)

Comment 68

[Kushagra Singh [:kushagra]]

Ryan, do you need to me to make any changes to the patch ?

Comment 69

[Ryan VanderMeulen [:RyanVM]]

I can't push it if it doesn't apply...

Comment 70

[Kushagra Singh [:kushagra]]

Attachment: revised_patch.patch

Sorry for the inconvenience. Please try this one. I rolled back the changes and created a new patch.

Comment 71

[Kushagra Singh [:kushagra]]

Attachment: revised_patch.patch

Sorry for the inconvenience. Please try this patch. I rolled back the changes and created a new patch.

Comment 72

[Kushagra Singh [:kushagra]]

ah, this is weird. The earlier patch did'nt appear and now it has.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8404804 [details] [diff] [review]
revised_patch.patch

Please just use checkin-needed.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/14bcda753898

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14bcda753898