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

Demonstrate unexpected timeout behaviour when sources are slow #35

Conversation

seddy
Copy link
Contributor

@seddy seddy commented Jun 18, 2018

Hello! I'm raising this to highlight an error that took me several days to debug, as it didn't seem to make any sense. I'm more than happy to work on a fix for this, but wanted to raise this PR to give a starting point for any discussions about it, as it's a bit of a weird one :)

The test here results in the following test error:

$ mix test
Compiling 1 file (.ex)
.................

  1) test fails silently and returns an empty map of sources if things timeout (DataloaderTest)
     test/dataloader_test.exs:22
     Assertion with == failed
     code:  assert dataloader == new_dataloader
     left:  %Dataloader{options: [timeout: 1], sources: %{test: %Dataloader.TestSource{batches: [], opts: [], results: %{}}}}
     right: %Dataloader{options: [timeout: 1], sources: %{}}
     stacktrace:
       test/dataloader_test.exs:31: (test)

Finished in 0.6 seconds
18 tests, 1 failure

Randomized with seed 153869

This demonstrates the fact that if a Source times out, we don't return
an error but rather just drop it from the map of sources in our
dataloader. The problem with this is that it leads to unexpected
behaviour when we hit a timeout.

Using the example from the moduledoc:

    # Normal setup
    source = Dataloader.Ecto.new(MyApp.Repo)
    loader = Dataloader.new |> Dataloader.add_source(:db, source)
    loader =
      loader
      |> Dataloader.load(:db, Organization, 1)
      |> Dataloader.load_many(:db, Organization, [4, 9])

    # It's entirely possible that the `Source` will timeout here, and if
    # so then this returns a loader that looks like `%Dataloader{sources: %{}}`
    loader = Dataloader.run(loader)

    # This call raises an error: `Source not found: :db`. This is
    # because there is no `:db` source anymore
    organizations = Dataloader.get_many(loader, :db, Organization, [1,4])

There are two issues to resolve here as I see it:

  1. We shouldn't be silently dropping sources if they timeout, we should
    handle that case in pmap and either raise or return an appropriate
    error tuple
  2. The default timeout for Dataloader should be slightly higher than
    the maximum default of all its sources, and it should be documented that
    if you set the timeout option, it must be higher than all sources,
    possibly even enforced

This results in the following test error:

    $ mix test
    Compiling 1 file (.ex)
    .................

      1) test fails silently and returns an empty map of sources if things timeout (DataloaderTest)
         test/dataloader_test.exs:22
         Assertion with == failed
         code:  assert dataloader == new_dataloader
         left:  %Dataloader{options: [timeout: 1], sources: %{test: %Dataloader.TestSource{batches: [], opts: [], results: %{}}}}
         right: %Dataloader{options: [timeout: 1], sources: %{}}
         stacktrace:
           test/dataloader_test.exs:31: (test)

    Finished in 0.6 seconds
    18 tests, 1 failure

    Randomized with seed 153869

This demonstrates the fact that if a `Source` times out, we don't return
an error but rather just drop it from the map of sources in our
dataloader. The problem with this is that it leads to unexpected
behaviour when we hit a timeout.

Using the example from the moduledoc:

    # Normal setup
    source = Dataloader.Ecto.new(MyApp.Repo)
    loader = Dataloader.new |> Dataloader.add_source(:db, source)
    loader =
      loader
      |> Dataloader.load(:db, Organization, 1)
      |> Dataloader.load_many(:db, Organization, [4, 9])

    # It's entirely possible that the `Source` will timeout here, and if
    # so then this returns a loader that looks like `%Dataloader{sources: %{}}`
    loader = Dataloader.run(loader)

    # This call raises an error: `Source not found: :db`. This is
    # because there is no `:db` source anymore
    organizations = Dataloader.get_many(loader, :db, Organization, [1,4])

There are issues to resolve here as I see it:

1. We shouldn't be silently dropping sources if they timeout, we should
   handle that case in `pmap` and either raise or return an appropriate
   error tuple
2. The default timeout for `Dataloader` should be slightly higher than
   the maximum default of all its sources, and it should be documented that
   if you set the timeout option, it must be higher than all sources,
   possibly even enforced
@seddy
Copy link
Contributor Author

seddy commented Jun 28, 2018

This PR is irrelevant since #36, closing

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.

1 participant