Skip to content

Discussion about behavior #7

Closed
Closed

Description

@jjb

I have this experimental improvement to timeout: https://github.com/jjb/better_timeout/. It's a refresh of an old project and I'm still exploring its goals. For the purpose of this discussion we can ignore the gem itself.

While making the gem I made some expanded tests for stdlib timeout.

Here's the code that runs for the expanded tests: https://github.com/jjb/better_timeout/blob/master/test/error_lifecycle.rb

This file (from the linked line down) shows behavior for ruby 2.4+, including current HEAD of ruby/timeout: https://github.com/jjb/better_timeout/blob/master/test/test_timeout-2.4-3.0.rb#L67

I have the scenarios described with a comment above each test_N. I have the behavior annotated with "expected", "weird", and "bad" (my opinion, and the topic of this discussion).

There are three things I'd like to discuss.

1. should an exception always be raised in outer?

In my opinion, Timeout.timeout should always raise an exception when it times out.

If we think it shouldn't in some scenarios, then I think that behavior should be controlled by the invocation of Timeout.timeout (in a new option), not by whether or not the inner code happens to rescue Exception

These tests seem to explicitly state that internal code should be allowed to swallow all exceptions, so I'm guessing I won't have any luck trying to convince this project to change 😅 https://github.com/ruby/timeout/blob/master/test/test_timeout.rb#L34-L60

I tried translating https://bugs.ruby-lang.org/issues/8730 but didn't understand the rationale behind enforcing the not-raise behavior.

2. weird behavior in test_2

In test_2, exception to raise is not specified, but inner code does rescue Exception. Regardless of this, the inner code is not able to rescue the error. I haven't yet looked into why this is.

3. are you interested in putting my tests into the test suite?

Are you interested in a PR which puts these tests into the ruby/timeout test suite? I'll redo them so that they are more in the style of the current tests, and try to rework them so that they don't use global variables, or at least use much safer namespacing if that's not possible.

If you got this far, thanks for reading! Let me know what you think!

Activity

  1. eregon commented on May 31, 2022

    @eregon
    Member

    There is some discussion in #14 (comment) regarding keeping the usage of throw or not to interrupt the timeout block.
    IMHO it'd be better to use Exception (or subclass of Exception), but I think that needs a new issue on https://bugs.ruby-lang.org/ to discuss and decide.
    It's unclear if it's OK to just use Timeout::Error inside the block because that's < RuntimeError, so it might be more likely to be caught accidentally.

    Yes, more tests are welcome.

  2. eregon commented on Jun 22, 2023

    @eregon
    Member

    Addressed by #30

  3. jjb commented on Jun 30, 2023

    @jjb
    ContributorAuthor

    😲

  4. jjb commented on Jun 30, 2023

    @jjb
    ContributorAuthor

    Interesting that that got reverted. Here's some stashed comments I have from when I was trying to understand this behavior 😄

    image image
  5. jjb commented on Jul 1, 2023

    @jjb
    ContributorAuthor

    Yes, more tests are welcome.

    #33

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions