kibertoad / toad-scheduler

In-memory Node.js and browser job scheduler
MIT License
564 stars 25 forks source link

Question: Meaning of memory leak when using async/await #197

Open milad2golnia opened 1 year ago

milad2golnia commented 1 year ago

You are giving a warning to don't use the async/await syntax inside the AsyncTask:

Note that in order to avoid memory leaks, it is recommended to use promise chains instead of async/await inside task definition.

I watched the video you mentioned but I still couldn't figure it out what you are concerning about? I think this warning is not the case when I'm scheduling a single job (or scheduling some few - less than 10 - jobs). So are you warning about executing many multiple tasks which are using async/await syntax simultaneously because each await is creating 3 promises in heap memory? It is appreciated if you give an example of how using async/await could results in memory leak. Personally, I prefer to use async/await syntax always because it is more readable and that's why I'm curious about your warning.

olsonpm commented 11 months ago

I also don't understand the warning - it feels wrong as I consider myself pretty knowledgeable in promises. Async/await is no different from promises and thus should behave the same regarding memory leaks

kibertoad commented 11 months ago

Please watch https://www.youtube.com/watch?v=XV-u_Ow47s0 It explains why callback and async functions shouldn't be mixed. You can do that as long as you handle rejections properly, of course.

olsonpm commented 11 months ago

I'm sorry but I'm not going to sit through a 30 minute video regarding a concept that I understand very well. Videos are a slow form of communication compared to written material. If you have a timestamp or a post somewhere that explains your point better then I'll take a look and discuss, but it sounds like you're warning against improper use of promises rather than async/await

kibertoad commented 11 months ago

It's not about async/await per se, but about mixing callbacks and promises. async/await is perfectly fine here as long as you are handling unhandled rejections properly, which you don't get for free when creating promises in a callback (it will not bubble up to your global error handler). toad-scheduler does its best not to crash your app if your promise throws (see https://github.com/kibertoad/toad-scheduler/blob/main/lib/common/AsyncTask.ts), but it's much better when there is explicit error handling on your end.

olsonpm commented 11 months ago

gotcha - thanks much

milad2golnia commented 10 months ago

You can do that as long as you handle rejections properly, of course.

So you should give a warning about unhandled rejections not memory leak, Am I right? Because unhandled rejections results in crash and not memory leak.