Try with timeout rethrow

[gustafsson]

The previous behaviour was to return any exception as res which I believe was a bug.

[quinnj]

Hmmm, can you clarify the issue you're seeing that's prompting the changes here? I'm not sure I follow your PR description details.

[gustafsson]

The wrapped function f in try_with_timeout may throw. For any reason outside the control of try_with_timeout.

When f throws an excpetion e and the condition is notified with notify(cond, e) we'll pick it up on the parent task with res = wait(cond). res will now refer to e which then gets returned with return res.

By changing notify(cond, e) to notify(cond, e, error = true) the exception will be rethrown on the parent task by res = wait(cond) instead.

Effectively this means that a pattern like this (for instance in sslupgrade) behaves the same with respect to exceptions from f.

res = if timeout > 0
        try_with_timeout(.., timeout, ..) do
            f(args...)
        end
    else
        f(args...)
    end

[codecov-commenter]

Codecov Report

Merging #942 (618dab2) into master (2377beb) will decrease coverage by 0.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
- Coverage   80.44%   79.98%   -0.46%     
==========================================
  Files          36       36              
  Lines        2950     2953       +3     
==========================================
- Hits         2373     2362      -11     
- Misses        577      591      +14     

Impacted Files	Coverage Δ
src/Exceptions.jl	94.59% <100.00%> (+3.41%)	⬆️
src/HTTP.jl	73.33% <0.00%> (-13.34%)	⬇️
src/clientlayers/RetryRequest.jl	75.60% <0.00%> (-2.44%)	⬇️
src/connectionpools.jl	72.72% <0.00%> (-1.02%)	⬇️
src/ConnectionPool.jl	86.69% <0.00%> (-0.86%)	⬇️
src/Messages.jl	86.39% <0.00%> (-0.60%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[gustafsson]

If I recall corretly I ran into this a while back when the connection was closed during sslupgrade and something in MbedTLS threw an exception that didn't get rethrown by try_with_timeout.

[quinnj]

Why do we need to check isopen(timer) here? It seems like it wouldn't matter whether it's open or not?

[gustafsson]

iftimeout() may cause f to throw. In that scenario I'd rather pick up the TimeoutError in wait(cond) and swallow the exception from f