Skip to content

Remove un-needed line in TimerTask #852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 16, 2020

Conversation

fzakaria
Copy link
Contributor

@fzakaria fzakaria commented Feb 13, 2020

Removed self.value = value in TimerTask#timeout_task

This line was probably added as a copy-mistake from the above
method (execute_task).

I ran brake spec SPEC=spec/concurrent/timer_task_spec.rb and the tests pass.
I can't see why the need to self-assign the variable here.

co-author: @kalenp

Removed `self.value = value` in TimerTask#timeout_task
This line was probably added as a copy-mistake from the above
method (execute_task).
@fzakaria
Copy link
Contributor Author

fzakaria commented Feb 13, 2020

Some more notes on TimerTask we are discussing:

  1. try? on event is confusing or rather has no documentation.
  2. there should be explicit mention that TimerTask is a fixed delay rather than fixed rate
    References to https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ScheduledFuture.html would be good here.
  3. If the delay + timeout_interval is shorter than the length of time the task takes; concurrent tasks will run.

@pitr-ch
Copy link
Member

pitr-ch commented Feb 16, 2020

Thank you for taking an interest. There is ongoing effort to rewrite TimerTask to something more simpler and maintainable #829 the code there is incomplete though, I have more changes locally.

@pitr-ch pitr-ch merged commit 50917ff into ruby-concurrency:master Feb 16, 2020
@pitr-ch
Copy link
Member

pitr-ch commented Feb 16, 2020

Please wait until I finish and push the changes I have. Let's address points you had on the new implementation.

@fzakaria
Copy link
Contributor Author

@pitr-ch I think you commented on the wrong PR

@pitr-ch
Copy link
Member

pitr-ch commented Feb 17, 2020

Actually I did not but the comment is not understandable 😁sorry
I meant to ask you not to invest more time in the TimerTask until I push the changes to the other PR, I am trying to prevent effort duplication.

@fzakaria
Copy link
Contributor Author

Gotcha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants