Add WebWorker Thread Support for Running Tasks #348
Conversation
This commit implements a task manager variant that uses worker threads. A new worker is spawned for each new task that appears. All functionality should work, but hasn't been fully tested at this time. The TaskThreadManager is added to the export list for use in the frontend.
THis commit fixes some various spelling typos found in the process manager files when implementing the task thread manager.
This commit updates the frontend to support using the task thread manager. The dev environment is adjusted to use it, but more testing should occur before it is used as the default. Further, a check was added to make sure worker threads are supported before using the TaskThreadManager. If the are not supported a warning message is printed and the TaskProcessManager is used instead.
pr1sm
added
type:enhancement
Not yet -- I did a test run of a single task and once I saw it "work", I opened the PR. By "work" I mean mostly work -- the status messages and captcha stuff worked, so I counted that as good enough for a WIP PR! |
Gotcha! So excited for this to hit beta 6 and for it to be rolled out. Good work 🙂 |
This commit addresses changes requested in PR: - TaskManager now has an Abort event defined. this is used instead of a string literal - TaskThreadManager now properly handles aborting on close - TaskThreadManager has changes to remove eslint errors - TaskThreadManager now checks for Webworker compatibliity in constructor. An error is thrown if there is no compatibility - TaskThreadManager and TaskProcessManager are refactored slightly to reduce code duplication - TaskManagerAdapter has better logic for falling back when worker threads are unavailable
This commit restructures manager files into their own folders. This helps visually group similar classes/files.
pr1sm
added
the
status:on hold
This commit consolidates shared code between the two scripts that ran on child contexts (either process or thread). Most the the code has been extracted out to a base class and sub classes have been created for each implementation.
|
Noticed a few bugs while playing around today. I'll do my best with the descriptions and repro steps. Stack Trace while deregistering task eventsSteps to reproduce
EnvironmentsDevelopment && Production Slack Trace
No log messagesI've encountered this a few times and can't tell you why it happens. Sometimes I'll launch the app and the log messages will be there, and sometimes they won't show up and will only output in the console. Steps to reproduce
EnvironmentsDevelopment && Production Stack TraceN/A |
This commit refactors similar code from the TaskProcessManager and TaskThreadManager into a new SplitContextTaskManager. This class is used as a base class for all future task managers that run in split contexts. Subclasses for processes and worker threads have been implemented and are now used by the frontend.
|
Thanks for testing this out! I've just finished some major refactors to how the managers work, so I'm not sure if I've fixed the errors. I'll continue to do some more testing and see if I can get those to occur on my end. Which OS were you testing on? |
|
No problem! I was testing on my Mac. |
|
I've been testing this out on Windows and I haven't seen either of those bugs. switching to macOS now. Are there any error messages that get printed to the console/logs when these errors occur? Both of these seem like they are related to the frontend side of things (in the |
Interesting... yeah I don't see any errors or anything. Just doesn't log properly to the frontend for me. Could it be my version of electron or something? Idk.. |
|
Yeah I always do a |
hmm still seems to be happening to me. If it's only me, idc as much. Just wanna make sure it's not a production issue. |
This commit fixes a bug that prevented tasks from starting due to a typo in the send proxy side effect handler.
This commit fixes a bug due to improper handling of the launcher ipc tunnel. When the tunnel closes, the launcher now does not attempt to send any messages, which prevents a crash like this from occurring.
This commit adjusts the logic surrounding registering for task events. Now the launcher will register for events as soon as the adapter is ready. This prevents any possibility of the task event chain not being set up properly to forward events to the frontend. This commit also hides console logs so they only show in dev mode and renames the manager.html file to a more appropriate launcher.html
This commit fixes a bug where deactivating the frontend would prevent the task launcher from starting again. A code path to launch the main window was missing the call to start the launcher as well.
We have a requirement to use instance methods within the Transformer subclasses, but the implementations don't use `this`. Since this is a valid exception to the rule, an eslint comment is added to the top of both to convey this. See https://eslint.org/docs/rules/class-methods-use-this#exceptions for more details
|
Seems like we get errors when trying to run at scale. I ran 50 tasks and got the following error message. The responsiveness of the app was also not that great. while running: |
This commit adjusts the max listeners for the TaskManager and TaskRunner event listeners. This allows many tasks (>25) to run without producing memory leak warnings.
|
Updating the max listeners helped, but I'm still seeing reduced responsiveness at ~50 tasks. |
|
Looks like the only issue was monitor/error delays aren't updating properly, other than that I'm trying to think of any other functionality and I can't.. |
|
👍 I'll take a look into fixing that. Does swapping proxies work? Just thought of that |
Lemme spin up a few proxies to test |
This commit updates the event handlers to allow responding to a bulk event. In this case the ID passed will be 'ALL'. This allows a single event to be emitted when adjusting a delay or webhook in the TaskManager for all tasks. This also keeps the contract of purely using event based communication between the manager and runner.
I also noticed this testing on my i7 sandy bridge e running 40 tasks. Looks like the app is spawning 2,000+ threads for only 40 tasks for some reason. |
This commit updates the default single thread TaskManager Implementation to have similar syntax as the SplitContextTaskManager. This ensures all TaskManger implementations work when used.
|
Whoa, that's crazy -- not sure why that's happening! |
|
ready for merge? @walmat |
|
Let me do a full run through one more time in dev and prod mode to make sure! @pr1sm |
|
@pr1sm we're good to go in my opinion! No hiccups. |
|
When I get back to my computer, I'll change the defaults to use the thread manager, then we can test one more time and merge |
This commit updates the default task manager to use the multi thread implementation. The set env var for prod is also updated to use thread instead of process.
|
@walmat Ok I think this is ready for a final review, test, and merge! |
All systems go for me! Tested all use cases in dev & prod. |
|
🚀 merging... |
Changes
TaskThreadManagerthat spawns worker threads for each taskrunnerThread.jsto handle passing events betweenTaskManagerandTaskRunnerusing theWebWorkerapiTaskThreadManagerin frontend by setting environment variableNEBULA_RUNNER_CONCURRENCY_TYPEtothreadto enable thisChecks
TaskThreadManagerfixes #192
Items left to completeAll Items CompleteGraceful cleanup on close. Closing the frontend does send a message to close workers, but they do not responddoneRefactor out shared parts ofdonerunner<x>.jsto reduce duplicate codeRefactor out shared parts ofdoneTask<x>Managerto reduce duplicate codeWhile writing thedone (see https://github.com/walmat/nebula/issues/192#issuecomment-466199341 for more details)TaskThreadManagerthere was so much duplicate code fromTaskProcessManager, the only significant changes I made were the functions I used to adhere to the webworker api. I want to refactor out this shared code so we don't have to change code in two places when dealing with bugs.