Reimplement http client timeouts using hashed wheel timer

[kachayev]

The motivation behind it was described here.

http/request now uses the default shared timer or provided as a parameter. On higher loads it makes sense to use different timeouts instead of a single instance even tho' hashed wheel timer is designed to handle thousands of simultaneous tasks with almost no visible performance degradation. It would be probably nicer to have timer being attached to a specific connections pool, but the current implementation does not let us attach arbitrary artifacts to flow/instrumented-pool... any other approaches would either break compatibility or would look weird.

@alexander-yakushev Please, take a look when you have time. Thanks!

[alexander-yakushev]

Looks good!

Do I understand correctly that it doesn't solve the issue that timeout callbacks could be slow/blocking, and will end up being executed by a single poor Timer thread?

Anyway, this is a strict improvement over what we have right now.

[kachayev]

@alexander-yakushev

it doesn't solve the issue that timeout callbacks could be slow

Why so? The timeout just realized the given deferred with an exception. The chain of callbacks should follow the same path it would follow otherwise. I don't see anything that might be blocking here... Am I missing something?

[kachayev]

@alexander-yakushev Would you be able to compare performance on some real-world use cases? If it works, I would update implementation with more fine-grained control over timer settings. Oh, and docs.

[ztellman]

Should this be defonce?

[kachayev]

@ztellman It definitely should! I'm waiting for the feedback from @alexander-yakushev if this even worse being changed. If so, I will complete the implementation and polishing.

[kachayev]

Done.

[ztellman]

I'd like delay to be delay-ms, just so it's clear what unit we're using without looking inside the implementation.

[kachayev]

Done.

[alexander-yakushev]

@kachayev I mean this scenario:

user=> (def my-d (d/deferred))
#'user/my-d
user=> (def other-process 
         (-> my-d 
             (d/catch identity) 
             (d/chain (fn [_] 
                        (println "Gonna block in" (Thread/currentThread)) 
                        (Thread/sleep 5000)))))
#'user/other-process
user=> (d/timeout! my-d 300)
<< … >>
Gonna block in #object[java.lang.Thread 0x6ac1e042 Thread[manifold-scheduler-pool-1,5,main]]

[kachayev]

@alexander-yakushev Should it be netty/timeout!?

[alexander-yakushev]

@kachayev That was an example of the current behavior, and your implementation is not different in this regard.

(netty/timeout! (create-timer) my-d 500)
<< … >>
Gonna block in #object[java.lang.Thread 0x77142860 Thread[aleph-timer--1,5,main]]

I brought this up because you mentioned this yourself here #479 (comment)

[kachayev]

Ah, okay. I thought you’re talking about some behavior specific to a new timer. What you have here is better scheduling optimized for a lot of simultaneous timeouts and the ability to start independent timer when you need to. I didn’t change the manifold’s semantic on purpose. First of all it’s generally a bad idea to block the thread if you’re not sure the you’ve launched this one. Which would be a narrative behind all async frameworks. Including Netty itself. You still can solve this problem using manifold controls, so I don’t think that’s such an a problem.

On Tue, Mar 26, 2019 at 01:20 Alexander Yakushev ***@***.***> wrote: @kachayev <https://github.com/kachayev> That was an example of the current behavior, and your implementation is not different in this regard. (netty/timeout! (create-timer) my-d 500) << … >> user=> Gonna block in #object[java.lang.Thread 0x77142860 Thread[aleph-timer--1,5,main]] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#499 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdpD5LeC-1SXkPeL56R5qT7FcU9eSjPks5vadhegaJpZM4cF2zW> .

-- Oleksii Kachaiev, CTO | Attendify M: +1-650-451-2271

[kachayev]

@ztellman Merged with the latest master. I also set tick duration to 10ms, to be more precise by default. I think it makes sense to manually tune this for higher loads than to keep it less precise for everyone.

[kachayev]

Even thought I still think there's a benefit of having timers optimized for highly loaded I/O environments, this specific implementation requires substantial portion of testing (ideally, in multiple production systems). Moreover, better approach would be to dispatch between timer types when necessary instead of using single timer for all needs. Happy to assist anyone who would be willing to extend the functionality. As of now, closing the PR.

[alexander-yakushev]

Sorry it slipped through the cracks for me, I totally forgot this PR exists. I never tested this particular PR because we need all Manifold timeouts to be using HWT. But we have been using HWT for this for the last 1.5 years in multiple high-load projects and haven't any issues so far.

Thanks for the effort!

[kachayev]

@alexander-yakushev Not a problem at all!