Smarter hibernate by redink · Pull Request #103 · elixir-ecto/db_connection

redink · 2017-11-20T07:43:09Z

fishcakez · 2017-11-20T15:43:32Z

+  alias TestPool, as: P
+  alias TestAgent, as: A
+
+  @tag :idle_hibernate


This test should have the :idle_timeout tag because it relies on idle timeout being active. The tags are used for test filtering, so if a pool doesn't support idle timeout it can't hibernate after an idle timeout. If there is a custom pool that needs this tag then we can leave it on. I think we should also move this test to the idle_test.exs file.

fishcakez · 2017-11-20T15:47:36Z

+    opts = [agent: agent, parent: self(), idle_timeout: 50, idle_hibernate: true]
+    {:ok, pool} = P.start_link(opts)
+    assert_receive {:hi, conn}
+    assert_receive {:pong, ^conn}


I wonder if we can test the process hibernates in someway.

fishcakez · 2017-11-20T15:48:30Z

 Code.require_file "cases/stream_test.exs", __DIR__
 Code.require_file "cases/transaction_execute_test.exs", __DIR__
 Code.require_file "cases/transaction_test.exs", __DIR__
+Code.require_file "cases/hibernate_test.exs", __DIR__


Alphabetically please because once we had a test file that wasn't being tested and that makes it easier to locate ;)

fishcakez · 2017-11-20T15:50:09Z

        done_lock(regulator, lock)
        {timeout, backoff} = Backoff.backoff(backoff)
-        {:backoff, timeout, %{s | lock: nil, backoff: backoff}}
+        maybe_hibernate(idle_hibernate, {:backoff, timeout, %{s | lock: nil, backoff: backoff}})


We should test this part with the other backoff handling

redink · 2017-11-21T01:56:13Z

I added a way for test the hibernate, I have no idea but only this one.

If you have better one, give me help please. Thanks.

fishcakez · 2017-11-21T02:13:13Z

@redink we could read the memory in the callback and send it in the message to the test process and then in a recursive loop wait for it to go down. Alternatively in a similar recursive loop we could wait for the Process.info(self(), current_function) to change to the appropriate function.

We should consider limiting the loop by a time limit using System.monotonic_time. Also we could sleep between each check to allow other processes to run.

fishcakez · 2017-11-21T06:52:41Z

    assert_receive {:error, conn}
    assert_receive {:hi, ^conn}

+    if test_hibernate? and Mix.env() != :sojourn do


If the :sojourn check is required then it means we are missing some code for the sojourn pool. We shouldn't have a Mix.env call, this context should be handled by tags or config.

We are missing the hibernate at this location:

db_connection/lib/db_connection/connection.ex

Line 544 in 03ea53f

continue_ask(%{s | idle_time: 0, state: state})

(i.e. after ping succeeds and the :sbroker async requests has been sent).

fishcakez · 2017-11-21T07:06:58Z

-    {:noreply, s, idle_timeout}
+  defp handle_timeout(%{client: nil, idle_timeout: idle_timeout,
+                        idle_hibernate: idle_hibernate} = s) do
+    Process.send_after(self(), :timeout, idle_timeout)


If we use a timer then we need to be careful to cancel the timer and/or have the timeout message uniquely identifiable (include reference) because the :timeout could arrive late and trigger early timeout and out of sync timeouts.

This module includes few examples of :erlang.start_timer to do this in similar way. Given timers have better behavior here (timers give stricter timeout than a timeout return value) its a good change. This might complicate code, I think that might be why didn't implemented like that originally.

Seems like we really need gen_statem here but cant until otp 20+.

fishcakez · 2017-11-21T07:08:21Z


+    if test_hibernate? and Mix.env() != :sojourn do
+      assert {:current_function, {:erlang, :hibernate, 3}} ==
+        Process.info(conn, :current_function)


This may have a race condition as we can't guarantee the process has hibernated because of the async nature of processes. However we can leave this until its observed.

fishcakez · 2017-11-21T07:10:52Z

+  end

-    opts = [agent: agent, parent: self(), backoff_min: 10]
+  defp execute_test_backoff_after_failed(agent, opts, test_hibernate? \\ false) do


If theres enough logic to share a function we could try to simplify the test when hibenating and not share function. We might have overlapping tests.

redink · 2017-11-21T16:19:16Z

+                        hibernate_timer: hibernate_timer} = s) do
+    cancel_timer(hibernate_timer)
+    hibernate_timer = Process.send_after(self(), :timeout, idle_timeout)
+    # hibernate_timer = start_timer(self(), idle_timeout, :trigger_hibernate)


NEED HELP !

The test will failed if I use start_timer, but it works well if I use Process.send_after/3.

The error information:

1) test ping after idle timeout using hibernate (TestIdle) integration_test/cases/idle_test.exs:15 No message matching {:pong, ^conn} after 500ms. The following variables were pinned: conn = #PID<0.561.0> The process mailbox is empty. code: execute_test_case(agent, opts, true) stacktrace: integration_test/cases/idle_test.exs:53: TestIdle.execute_test_case/3 integration_test/cases/idle_test.exs:18: (test)

When the timer arrives we need to call the logic that is currently done by handle_info(:timeout, ...) because the timer is replacing that timeout. The reason it fails is because we don't call apply(mod, :ping, [state]) anymore.

fishcakez · 2017-11-21T16:46:19Z

  defp cancel_timer(timer) do
    case :erlang.cancel_timer(timer) do
-      false -> flush_timer(timer)
+      false -> :erlang.read_timer(timer) and flush_timer(timer)


read_timer will always be false here so wont flush, we don't need this part.

fishcakez · 2017-11-21T16:49:54Z

  defp start_timer(pid, timeout) do
    :erlang.start_timer(timeout, self(), {__MODULE__, pid, timeout})
  end
+  defp start_timer(_, :infinity, _), do: nil


Lets reuse the existing timer function by calling start_timer(:idle, timeout) so the message carries some information!

fishcakez · 2017-11-21T16:50:23Z

    receive do
      {:timeout, ^timer, {__MODULE__, _, _}} ->
        :ok
+      {:timeout, ^timer, :trigger_hibernate} ->


We won't need this clause see above!

fishcakez · 2017-11-21T16:51:19Z


    s = %{mod: mod, opts: opts, state: nil, client: :closed, broker: broker,
          regulator: regulator, lock: nil, queue: queue, timer: nil,
+          hibernate_timer: nil,


Please use idle_timer as the timer is for idle and not hibernating, but hibernate is a possible side effect of going idle.

fishcakez · 2017-11-21T16:52:52Z

    execute_test_backoff_after_failed(agent, opts)
  end

+  @tag :idle_hibernate_backoff


We should not require this tag, all pools should support this.

Actually lets keep this for now, ignore the above comment.

fishcakez · 2017-11-21T16:53:10Z

  end

-  @tag :idle_timeout
+  @tag :idle_hibernate


We should support this for all pools that can do idle_timeout, I left a comment on how to achieve this previously.

Actually lets keep this for now, ignore the above comment.

fishcakez · 2017-11-21T16:56:46Z

+                        hibernate_timer: hibernate_timer} = s) do
+    cancel_timer(hibernate_timer)
+    hibernate_timer = Process.send_after(self(), :timeout, idle_timeout)
+    # hibernate_timer = start_timer(self(), idle_timeout, :trigger_hibernate)


When the timer arrives we need to call the logic that is currently done by handle_info(:timeout, ...) because the timer is replacing that timeout. The reason it fails is because we don't call apply(mod, :ping, [state]) anymore.

fishcakez · 2017-11-21T16:58:23Z

  end
  def handle_info(:timeout, %{idle_hibernate: idle_hibernate} = s) do
-    maybe_hibernate(idle_hibernate, {:noreply, s})
+    maybe_hibernate(idle_hibernate, {:noreply, %{s | hibernate_timer: nil}})


We should never reach this handle_info/2 clause. I think we can delete this clause.

fishcakez · 2017-11-21T17:00:05Z


+  def handle_info({:timeout, timer, :trigger_hibernate},
+                  %{idle_hibernate: idle_hibernate} = s) when is_reference(timer) do
+    maybe_hibernate(idle_hibernate, {:noreply, %{s | hibernate_timer: nil}})


We shouldn't hibernate here, first we need to call the current logic in handle_info(:timeout, ..) that follows this clause: https://github.com/elixir-ecto/db_connection/pull/103/files/5b3c7cbd2333f15b8ca30bd06bcd9533820c2db0..e1590494cd81e376261aa7d9592a709c13028e85#diff-794f06ecb2c5f02e8a11e735c3ee2339R364

fishcakez · 2017-11-21T17:05:58Z

@redink I left some comments: the main takeaway is that the timer is for idling and for hibernating. The hibernation occurs after the idle handling (calling the ping callback), and it is possible we won't hibernate. The new option is to go idle after we ping or after we fail to connect.

redink · 2017-11-21T17:56:17Z

sojourn test still have problem, I will work on it later.

fishcakez · 2017-11-21T18:29:39Z

@redink the last changes look good. It might be that Sojourn doesn't play well with hibernate because of how the pool works. I suggest we don't implement hibernate for it yet and add the @idle_hibernate tag for sojourn tests.

redink · 2017-11-27T08:12:37Z

Hello, Any updates ? Or any more comments ?

fishcakez · 2017-11-27T17:25:26Z

@redink there is a performance regression so need to look in it.

fishcakez · 2018-01-07T22:35:31Z

Closing in favor of #108.

redink added 3 commits October 24, 2017 18:25

only hibernate when backoff and timeout

471f190

use idle_hibernate as pure options

2e065e6

add test case for idle hibernate

e658003

fishcakez reviewed Nov 20, 2017

View reviewed changes

fishcakez mentioned this pull request Nov 20, 2017

add hibernate option for long lived process #101

Closed

redink added 2 commits November 21, 2017 09:16

update unit test via cr

dbb2368

add test for hibernate

650be16

update unit test for hibernate

5b3c7cb

fishcakez reviewed Nov 21, 2017

View reviewed changes

use tag rather than Mix.env and add hibernate timer

e159049

redink commented Nov 21, 2017

View reviewed changes

fishcakez reviewed Nov 21, 2017

View reviewed changes

modified via cr

95d0a58

disable idle hibernate test when sojourn

3144264

fishcakez closed this Jan 7, 2018

Conversation

redink commented Nov 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redink commented Nov 21, 2017

Uh oh!

fishcakez commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fishcakez Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fishcakez commented Nov 21, 2017

Uh oh!

redink commented Nov 21, 2017

Uh oh!

fishcakez commented Nov 21, 2017

Uh oh!

redink commented Nov 27, 2017

Uh oh!

fishcakez commented Nov 27, 2017

Uh oh!

fishcakez commented Jan 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fishcakez Nov 21, 2017 •

edited

Loading