Promise support

[mastilver]

I can use your module for a database related module
Do you see any drawback adding support for Promises, let me know what you think!

I'm already working on it, it should be ready by the end of the day

[andywer]

The only pitfall I currently see is that you might need to run the promises sequentially in order to get clean heap snapshots. (There is only one heap, so running multiple stuff concurrently would make it impossible to get useful heap diffs, I guess)

[mastilver]

The only pitfall I currently see is that you might need to run the promises sequentially

@andywer yeah that's the plan

[mastilver]

@andywer I just realize I need to modify the api because if we want to support promise, we need to add a new function for Promise only

Several solutions:

1. keep iterate and add iterateAsync
2. rename iterate to sync, export the Promise based one as the default

const iterate = require('leakage')
iterate(100, () => {}) // returns Promise
iterate.sync(100, () => {}) // will throws if leaking

3. ...

I prefer number 2 as will work for both sync and async code, and if user really want only sync they can use it
It's a breaking change but the package is 0.x so it's not that big of a deal

//cc @btmills

[andywer]

@mastilver I don't think so. The only problem is that you cannot do iterate(100, async () => { ... }) because the promise will have been created (and thus started) before iterate() can do anything.

How about iterate(100, () => async () => { ... })? Not super elegant, but would work, I think.

Alternatively we might use data.task. Should basically work like a Promise, but it doesn't start on creation.

[mastilver]

How about iterate(100, () => async () => { ... })?

I don't see how you want to make it work, when the input function return a Promise, it needs to return a Promise as well, so we can wait for the result.

With solution 2: this would totally work

iterate(100, async () => {
  await someAsyncStuff()
})
.then(() => {
  // everything went well
})
.catch(err => {})

as well as

iterate(100, () => {
  someSyncStuff()
})
.then(() => {
  // everything went well
})
.catch(err => {})

[andywer]

Ok, my iterate(100, () => async () => { ... }) proposal only works if you assume that it's async if a function is returned (no matter if the returned function actually is async [= returns a Promise]).

But I would try to avoid having two different methods. I think the way Mocha or Jest do it (the function returns a promise or not), for example, is quite nice. We cannot go the exact same way if we want to run the tests strictly sequentially, but maybe can come up with something quite similar :)

Another easy solution might be iterate(100, (done) => { ... }) (node-style callback). But that feels so 2014... ^^

[mastilver]

But that feels so 2014... ^^

😆

But I would try to avoid having two different methods.

I don't particularly agree with that, the most majority of packages have a async fn by default and a sync if needed (ei: https://github.com/sindresorhus/execa)

I think the way Mocha or Jest do it (the function returns a promise or not), for example, is quite nice.

Yes, but they are the consumer, so they can do whatever they want (if they receive a Promise => wait until it resolve, if they receive a callback => same they wait, if it's synchronous => they just run it
Here leakage output is getting consumed by mocha/jest so it need a way to tell them it's done

So if we give Promises to leakage it needs to return a Promise as well
Or as you suggested, if we give callbacks to leakage it need to call a callback as well to notify mocha/jest

[andywer]

Yes, but they are the consumer, so they can do whatever they want

Hmm, iterate is consumer as well as producer. It runs the user's iterator function (consumer) and would return a promise for the complete iteration + heap analysis if async (producer).

But your point is valid. And there is another issue: I am not sure if we can really ensure that test runners run (start) async tests strictly sequentially: http://stackoverflow.com/a/12983519

Maybe we should even consider dropping official support for all test runner that we know we cannot guarantee sequential execution under all circumstances.

What do you think?

Poking @btmills as well.

[mastilver]

iterate is consumer as well as producer.

👍

I am not sure if we can really ensure that test runners run (start) async tests strictly sequentially: http://stackoverflow.com/a/12983519

Damn... It makes senses but I didn't think about that... 😞

[andywer]

Don't worry, that's still a normal part of the process. Let's take a step back, have a good night's sleep and come up with an elegant concept. Then we start thinking how to implement it :)

[btmills]

When designing an API, I like to write code as if I'm using my ideal API, and then figure out how to implement it. So here's some sketching:

1. Synchronous API as it is today. Keep this unchanged if possible.

   iterate(iterations: number, iterator: () => void): void

   it('leaks', () => {
     const leaks = []
   
     iterate(1000, () => {
       leaks.push(JSON.parse(fs.readFileSync('foo.json', 'utf-8')))
     })
   })

2. First idea for a callback-based asynchronous API. This passes Mocha's done callback to iterate, and iterate passes its own done callback to the iterator function.

   iterate(iterations: number, iterator: (done: (err?: any) => void) => void, done: (err?: any) => void): void

   it('leaks async', (done) => {
     const leaks = []
   
     iterate(1000, (done) => {
       fs.readFile('bar.json', 'utf-8', (err, data) => {
         if (err) return done(err)
   
         leaks.push(JSON.parse(data))
         done()
       })
     }, done)
   })

3. Second idea for a callback-based asynchronous API. iterate passes a done callback to the iterator function, but it returns a Promise to Mocha. It's less typing than (2), but the juxtaposition between promises and callbacks feels gross.

   iterate(iterations: number, iterator: (done: (err?: any) => void) => void): Promise

   it('leaks async via promise', (done) => {
     const leaks = []
   
     return iterate(1000, (done) => {
       fs.readFile('bar.json', 'utf-8', (err, data) => {
         if (err) return done(err)
   
         leaks.push(JSON.parse(data))
         done()
       })
     })
   })

4. Promise API. The iterator function returns a Promise to iterate, and iterate returns a Promise to Mocha.

   iterate(iterations: number, iterator: () => Promise): Promise

   it('leaks promise', () => {
     const leaks = []
   
     return iterate(1000, () =>
       fetch('/api/foobar')
       .then((response) => response.json())
       .then((data) => {
         leaks.push(data)
       })
     })
   })

I believe all of these forms could be differentiated inside a single iterate method by inspecting the length of the iterator function and the type of the iterator function's return value (typeof ret === 'object' && typeof ret.then === 'function').

Thoughts?

---

Maybe we should even consider dropping official support for all test runner that we know we cannot guarantee sequential execution under all circumstances.

I think that makes sense. Make it the user's responsibility (and note this in the docs) to ensure that there's nothing else happening concurrently inside the same process while a leak test is being run.

[andywer]

Yeah, 1 + 4 would also be my favorite. Combining it with a narrow set of supported test runners to ensure sequential testing should do the trick :)

Btw: I was already thinking about implementing a small check in iterate to throw if two iterate() are run concurrently. If you would use an unsupported test runner you would at least get an error stating that something's wrong with the setup. But only makes sense for async iterators.

[andywer]

Have been playing around with a solution for Promise support, so that iterate() can still work synchronously, but also handles asynchronous iterator functions properly and returns a promise.

Here are my experiences so far:

• In order to make it still work synchronously with synchronous iterator functions I had to come up with a hack: Promises normally never resolve synchronously (not even when doing Promise.resolve()), so I had to overload iterate()'s promises in a way that they can resolve synchronously
• Because of that I could not use async/await, because I needed to use my hacky promises
• All that promise handling causes additional heap manipulations which is not good

So promises may be nice for the public API, but internally iterate() should work callback-based in order to prevent heap modifications.

[stephenmathieson]

Any news here? I'd love to see promise support added. May even give it a shot if the API has been agreed upon and you're too busy.

[andywer]

@stephenmathieson Yeah, sorry that it takes rather long and sure, knock yourself out if you want 😉, but the reason why it takes rather long is quite simple:

Writing code for the iterate method is not like writing any other code. You must write the code that is run in between heap snapshots in a way that it does not manipulate the heap (no modifications at all if possible; no creation of objects, arrays or dynamic functions) in order to not pollute the heap snapshots. For the async version it is quite tricky, since both your hands must stay tied to your back.

Another thing is that I am currently also working on the next release of webpack-blocks.
If you can come up with a solution that does not interfere with the heap snapshotting, feel free to open a PR of course :)

I will work on it as well, of course, but it might take one or two weeks.

[andywer]

PS: Sorry for the long monolog, but I know it's a critical feature and it's good that you asked, so I know people care 😉

[mjhea0]

any updates on this?

[andywer]

Not yet, sorry. I experimented, but got no usable outcome yet. I am rather busy preparing webpack-block's biggest release right now... 😕

[mjhea0]

Can I see what you tried?

[andywer]

@mjhea0 Sure! I pushed it to branch async-support-2. When you run node test.js you will see that it throws a leak error, even though there is no leak...

[andywer]

Update: I tried another way of implementing async support in branch async-support.

The problem is: It failed on the first run and then started to work without any changes. So now all tests (including async ones) are green locally on my machine, but the travis build is mostly red (run tests on 3 node versions, 2 failed).

Not sure why...

[mjhea0]

Strange. Yeah, it passed on Node 7. Node probably just rejects them now.

http://stackoverflow.com/questions/20068467/do-never-resolved-promises-cause-memory-leak

[andywer]

It really seems to be node 7 vs. node < 7. I can reproduce it locally by switching between node versions.

But the only difference in node 7 promise handling I can find is that unhandled promise rejections show a warning.

I don't think it's about never resolving promises, since in the failing test case I wouldn't know where there is a single unresolved promise...

[andywer]

Good news, though. Tried it in a real world scenario using node 7 and worked just fine 🎉

Here is a small test setup testing the threads package before and after a memory leak fix: gist

[mjhea0]

Nice! Breaks on my end as well!

[andywer]

@mjhea0 It breaks on your machine... using node <= 6?

[mjhea0]

Sorry, I meant it is not breaking.

[andywer]

Just released version 0.3.0-beta with promise support. Please try it out if you can :)

It's really simple to use!

[andywer]

Known issues: You need node >= 7.0 and for some reason the tests fail on travis right now :-/

(Tests are run twice, first on 'prepublish' which is invoked by yarn, then a 2nd time as npm test. First time it was green, second time it failed. Happened two independent times on travis. Works locally)

[philkunz]

Whats the status here? :D

[andywer]

Status is: There is a solution for async testing in the release branch, but it randomly fails to do its job here and there and I can only guess why, but have no exact idea what's the problem 🙈💩

[philkunz]

Well, at least there is progress. And btw -> awesome package overall. Node needs something like this badly :D

[andywer]

Closing it in favor of #22. The next release supporting async testing is straight ahead!