Skip to content
This repository has been archived by the owner on Sep 28, 2024. It is now read-only.

Don't start a new thread for each runLater(duration) #836

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

mslenc
Copy link
Contributor

@mslenc mslenc commented Nov 7, 2018

In the current implementation, each call to runLater(duration) creates a new Timer, which in turn creates a new Thread. The threads never seem to get cleaned up, and eventually the process runs out of them.

In the current implementation, each call to runLater(duration) creates a new Timer, which in turn creates a new Thread. The threads never seem to get cleaned up, and eventually the process runs out of them.
@mslenc
Copy link
Contributor Author

mslenc commented Nov 7, 2018

Actually, looking at Timer source, it should eventually clean up after itself, but relies on finalize() to do so. I guess the error I saw was the result of my app doing no/very little memory allocation, while calling runLater every 0.2 seconds..
In any case, it's far more efficient to just use a single Timer instance..

@edvin
Copy link
Owner

edvin commented Nov 8, 2018

I agree if you use this a lot it would be better to create the timer once, though if you need to call something like this regularly you should probably not dispatch new jobs all the time, but rather write your own loop around it. If you could rename the timer variable to something more specific I will merge :)

@mslenc
Copy link
Contributor Author

mslenc commented Nov 8, 2018

Well, it's your name, I just moved the var - 4f5008e :)

Anyway, any suggestion on what the name should be? runLaterTimer?

@edvin
Copy link
Owner

edvin commented Nov 8, 2018

There is a huge difference between a local variable in a function, and a package level variable, even if it's private :) Yeah, that would be fine.

@edvin edvin merged commit 2070ee9 into edvin:master Nov 8, 2018
@edvin
Copy link
Owner

edvin commented Nov 8, 2018

Perfect, thank you!

@mslenc
Copy link
Contributor Author

mslenc commented Nov 8, 2018

sorry, I missed the other reference (FXTimerTask(op, timer) should be FXTimerTask(op, runLaterTimer))

@mslenc
Copy link
Contributor Author

mslenc commented Nov 8, 2018

should I open another pull request?

@edvin
Copy link
Owner

edvin commented Nov 8, 2018

Nah, I'm fixing it and committing now :)

@mslenc
Copy link
Contributor Author

mslenc commented Nov 8, 2018

OK, thanks :)

edvin pushed a commit that referenced this pull request Nov 8, 2018
@mslenc mslenc deleted the patch-1 branch November 8, 2018 10:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants