Skip to content

Conversation

@wintama-roberto
Copy link

@wintama-roberto wintama-roberto commented Oct 17, 2025

FixedRateTask using the time unit converted initial delay


Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

  1. Was this PR correctly labeled, did the right tests run? When did they run?
  2. Is this PR squashed?
  3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
  4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

@mbien mbien added the Platform [ci] enable platform tests (platform/*) label Oct 17, 2025
@mbien mbien added this to the NB29 milestone Oct 17, 2025
@mbien mbien added the ci:all-tests [ci] enable all tests label Oct 17, 2025
@apache apache locked and limited conversation to collaborators Oct 17, 2025
@apache apache unlocked this conversation Oct 17, 2025
@mbien
Copy link
Member

mbien commented Oct 17, 2025

hmm. the missing conversion was there since the code was introduced (archive).

Enabled all tests and targeted NB 29. Although this looks correct (on first glance, both ::reshedule impls of FixedRateTask and FixedDelayTasks expect ms), this could have side effects in case something compensated for it and indirectly relies on this bug.

(Before this can be merged, you would have to update your author and email on the commit though)

@mbien mbien removed the ci:all-tests [ci] enable all tests label Oct 17, 2025
@wintama-roberto
Copy link
Author

Yes, I was also surprised that this bug was not detected for so long time.
Maybe because for the first execution the delay is correct, as it will be scheduled here:
cfe7e0b#diff-772c41d04de094a38f69e278dbe23008634fc40b89f589d4f9ce4fef091eb393R1036
But after that it will be always executed twice, for a while (until the calculation is correct again). Which is - I assume - for most implementations worse than just having a shorter first delay.

If I’d compensate for the bug, I would have just used milliseconds for the timeUnit and both values. But of course, there is no guarantee someone just converted the initialDelay argument to milliseconds and passed another time unit value to the delay. Nevertheless, I think the bug should be fixed, as anyone would expect that it behaves like documented.

I directly commited in github, I hoped github will add the author and mail to the commit automatically. Let me check...

@mbien
Copy link
Member

mbien commented Oct 17, 2025

I directly commited in github, I hoped github will add the author and mail to the commit automatically. Let me check...

We basically require a full author name and a valid email address for contributions. The noreply mail is caused due to github settings which allow github to hide your mail.

you can check it by looking at the patch file of the commit:

https://github.com/apache/netbeans/pull/8932.patch

to fix this, you will have to amend the commit and force push into the PR branch. (but first check if it looks ok locally with git log)

FixedRateTask using the time unit converted initial delay
@wintama-roberto
Copy link
Author

Yes, it was private. I have updated the commit author and mail.

@mbien
Copy link
Member

mbien commented Oct 19, 2025

will probably merge in ~ a week or so. giving others time to chime in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Platform [ci] enable platform tests (platform/*)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants