Skip to content
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

fix #1251 Add explicit defer parameter to VirtualTimeScheduler.advanceTime_ methods #2012

Closed
wants to merge 1 commit into from

Conversation

yarosla
Copy link
Contributor

@yarosla yarosla commented Jan 11, 2020

No description provided.

@pivotal-issuemaster
Copy link

@yarosla Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@yarosla Thank you for signing the Contributor License Agreement!

@simonbasle simonbasle changed the title Issue-1251: add explicit defer parameter to VirtualTimeScheduler.advanceTime_ methods fix #1251 Add explicit defer parameter to VirtualTimeScheduler.advanceTime_ methods Jan 13, 2020
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I don't think this approach works, because it puts the empty check outside the drain() loop, which can be reached by other path than advanceTime. there can also be a situation where 2 parallel calls to advanceTime are made, with different options (although that should be way more rare). All in all, putting this parameter at the method level makes things more brittle and harder to reason about.

Given that, another possible approach would be to create the VirtualTimeScheduler with a final defer option. In the drain(), each !queue.isEmpty() check would also be checking if defer == false

You also went against my requirement that the defer mode had a default of true.

@yarosla
Copy link
Contributor Author

yarosla commented Jan 13, 2020

Your requirements were:

  • VirtualTimeScheduler defaults to defer = false
  • StepVerifier.withVirtualTime defaults to defer = true

I will look into this closer and make changes next weekend.

@simonbasle
Copy link
Member

@yarosla ah indeed, my bad. yeah please explore the alternative solution of setting this once and for all at VTSheduler creation when you have time 👍

@yarosla
Copy link
Contributor Author

yarosla commented Jan 19, 2020

Created new pull request #2017

simonbasle pushed a commit that referenced this pull request Jan 22, 2020
By default, VTS created manually don't defer the advancing of time
anymore, even if task queue is empty.

Those created implicitly by StepVerifier still do however, to
maintain the behavior in the case where deferring advanceTime is most
likely to be necessary.

See-Also: #2012
@simonbasle
Copy link
Member

forgot to close that one, since #2017 has superseded it

@simonbasle simonbasle closed this Feb 3, 2020
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.

None yet

3 participants