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

TCK: Receptacle#expectError timeout approach #452

Merged
merged 4 commits into from May 6, 2019

Conversation

Scottmitch
Copy link
Contributor

Motivation:
defaultTimeoutMillis is typically used as a "max time to wait until an expected event occurs". For example this value is used as the argument to a ArrayBlockingQueue#poll(timeout) [1][2][3] method call and therefore when things work as expected you rarely wait this full duration. However Receptacle#expectError does a Thread.sleep(..) and then checks if there is something present in the error list (which is a CopyOnWriteArrayList). The value for the sleep is typically derived from defaultTimeoutMillis which leads to unconditional delays. This makes it challenging to use defaultTimeoutMillis in tests to specify "maximum amount of time to wait until some event occurs". The impacts are if defaultTimeoutMillis is set very small you are more likely to see timeouts (especially on over utilized CI servers), but if defaultTimeoutMillis the tests take a long time to complete.

[1] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L1019
[2] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L978
[3] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L990

Modifications:

  • TestEnvironment now supports an additional poll timeout parameter to be used
    when there is no explicit async event to trigger off of.

Result:
TestEnvironment's defaultTimeoutMillis can be used as "max time until some event
occurs" where defaultPollTimeoutMillis provides the polling interval to check
for an event if there is no explicit async event to trigger off of.
Fixes #451

Motivation:
`defaultTimeoutMillis` is typically used as a "max time to wait until an expected event occurs". For example this value is used as the argument to a `ArrayBlockingQueue#poll(timeout)` [1][2][3] method call and therefore when things work as expected you rarely wait this full duration. However [Receptacle#expectError](https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L1030) does a `Thread.sleep(..)` and then checks if there is something present in the error list (which is a [CopyOnWriteArrayList](https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L45)). The value for the sleep is typically derived from [defaultTimeoutMillis](https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L521-L526) which leads to unconditional delays. This makes it challenging to use `defaultTimeoutMillis` in tests to specify "maximum amount of time to wait until some event occurs". The impacts are if `defaultTimeoutMillis` is set very small you are more likely to see timeouts (especially on over utilized CI servers), but if `defaultTimeoutMillis` the tests take a long time to complete.

[1] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L1019
[2] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L978
[3] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java#L990

Modifications:
- TestEnvironment now supports an additional poll timeout parameter to be used
  when there is no explicit async event to trigger off of.

Result:
TestEnvironment's defaultTimeoutMillis can be used as "max time until some event
occurs" where defaultPollTimeoutMillis provides the polling interval to check
for an event if there is no explicit async event to trigger off of.
Fixes reactive-streams#451
@Scottmitch
Copy link
Contributor Author

I have updated the CopyrightWaiver.txt as part of #450

@ktoso
Copy link
Contributor

ktoso commented Apr 24, 2019

Thanks Scott! Seems I'll be able to review this, yay! I'll give it a look now 👍

@viktorklang
Copy link
Contributor

Thanks for opening this PR @Scottmitch!

@reactive-streams/contributors Please chime in!

@ktoso
Copy link
Contributor

ktoso commented Apr 24, 2019

The new config has to be able to be configured from the env variables, as some people rely on those -- commented in line how to do it.

And the param has to be documented as well in the tck/README.md

Note that the TCK differentiates between timeouts for "waiting for a signal" (`defaultTimeoutMillis`),
and "asserting no signals happen during a given amount of time" (`defaultNoSignalsTimeoutMillis`).
While the latter defaults to the prior, it may be useful to tweak them independently when running on continuous integration servers (for example, keeping the no-signals timeout significantly lower). Another configuration option is the "poll timeout" which is used whenever an operation has to poll for a `defaultTimeoutMillis` for a signal to appear (most often errors), it can then poll and check using the `defaultPollTimeoutMillis` (which MUST be smaller than `defaultTimeoutMillis`), for the expected error, rather than blocking for the full default timeout.

In order to configure these timeouts (for example when running on a slow continuous integration machine), you can either:

**Use env variables** to set these timeouts, in which case the you can do:

    export DEFAULT_TIMEOUT_MILLIS=100
    export DEFAULT_NO_SIGNALS_TIMEOUT_MILLIS=100
    export DEFAULT_POLL_TIMEOUT_MILLIS=20
    export PUBLISHER_REFERENCE_GC_TIMEOUT_MILLIS=300

Along the way there's some typos there we could fix (the above text should be fine to copy verbatim if you like it @Scottmitch :)

Sadly we have the readme in two places, since JDK and RS versions, so this would also have to be done in: https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck-flow/README.md

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

I think this looks promising, needs some final polish though to "fit in" into the existing infra 👍
Thanks a lot @Scottmitch!

tck-flow/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

This looks good now, thanks for the explanations and adding the ENV support @Scottmitch!

LGTM!

@Scottmitch
Copy link
Contributor Author

thanks for review @ktoso and @viktorklang! Any other comments, or can this be merged?

@ktoso
Copy link
Contributor

ktoso commented May 5, 2019

Looks good as I said above here. Ping @viktorklang. Sadly I never got merge powers here... ¯_(ツ)_/¯

@viktorklang
Copy link
Contributor

@Scottmitch Thanks for the great contribution!

@ktoso Thanks for helping out reviewing, Konrad! ^^
I'll investigate if we can give you merge-powers, it seems like an oversight.

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.

TCK: Receptacle#expectError timeout approach
3 participants