Start additional threads only when necessary - #415
Conversation
| self.logger.error(msg) | ||
| raise TelegramError(msg) | ||
|
|
||
| self._init_async_threads(uuid4(), self.workers) |
There was a problem hiding this comment.
I think I agree with this change, but I'd like @tsnoam to give it a look as well.
| @@ -51,10 +49,6 @@ def __init__(self, bot, prevent_autostart=False): | |||
| """:type: float""" | |||
| self._running = False | |||
|
|
|||
| self.start() | ||
|
|
||
| def put(self, job, next_t=None): | ||
| """Queue a new job. If the JobQueue is not running, it will be started. |
There was a problem hiding this comment.
This docstring is wrong (and was wrong before your PR). The job queue is never started in this method. Please remove that sentence.
| self.running = True | ||
|
|
||
| # Create & start threads | ||
| self._init_thread(self.job_queue.start, "job_queue") |
There was a problem hiding this comment.
Here you can simply call self.job_queue.start(), I believe. The method is already spawning a new thread.
There was a problem hiding this comment.
+1 @jh0ker comment here.
@wronglink can you please fix?
| self.running = True | ||
|
|
||
| # Create & start threads | ||
| self._init_thread(self.job_queue.start, "job_queue") |
|
Hey, thanks! I did a short code review. As mentioned in the review, I am hereby pinging @tsnoam to check out the code as well ;) Edit: I'm putting this up for the 6.0 milestone because of the removal of the |
tsnoam
left a comment
tsnoam
left a comment
There was a problem hiding this comment.
@wronglink
All in all looks great. It makes perfect sense to do it that way, thankyou for your contribution.
One small thing, please fix according to @jh0ker comment on updater.py, line 160. Then we can merge.
| self.running = True | ||
|
|
||
| # Create & start threads | ||
| self._init_thread(self.job_queue.start, "job_queue") |
There was a problem hiding this comment.
+1 @jh0ker comment here.
@wronglink can you please fix?
|
for some reason, i couldn't see the sources with decreased coverage. i'm attempting rebuild with travis, maybe it'll help. |
|
@tsnoam I've merged master branch, so forced TravisCI to recalculate coverage. Now files with decreased coverage are showed on coveralls. |
|
@wronglink |
Bibo-Joshi
added
🔌 enhancement
4 participants
I have several troubles with
JobQueueandDispatcherautomatically start new threads during it's initialisation. I guess it's quite unexpected behaviour to have a branch of threads just by creating newUpdater, without starting it. Instead of it, I propose theUpdaterinstance should start all necessary threads on inside it's start methods:start_pollingandstart_webhook.