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

Use Promises for async api calls #51

Merged
merged 1 commit into from Jun 21, 2022
Merged

Use Promises for async api calls #51

merged 1 commit into from Jun 21, 2022

Conversation

will
Copy link
Collaborator

@will will commented Jun 18, 2022

The first try at manually using spawn and channels worked, but if there
was an exception raised, it'd print a stack trace like this instead of
being handled nicely

Unhandled exception in spawn:  (CB::Client::Error)
  from src/cb/client.cr:517:5 in 'exec'
  from src/cb/client.cr:484:5 in 'get'
  from src/cb/client.cr:144:5 in 'get_teams'
  from src/cb/completion.cr:126:22 in '->'
  from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:146:11 in 'run'
  from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:98:34 in '->'
Unhandled exception in spawn:  (CB::Client::Error)
  from src/cb/client.cr:517:5 in 'exec'
  from src/cb/client.cr:484:5 in 'get'
  from src/cb/client.cr:144:5 in 'get_teams'
  from src/cb/client.cr:234:18 in 'get_clusters'
  from src/cb/completion.cr:128:22 in '->'
  from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:146:11 in 'run'
  from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:98:34 in '->'

The way to get around this would be to have the channel also be able to
take an exception, and resuce exceptions in the spawn block and send
them on the channel, then raise them in the main thread. This is a lot
of boilerplate work. I started writing a thing to do this for us, then
realized it was basically the promise pattern, and that someone else
probably has done that. I was a little hesitant to pull in this library
but it's not that large, and seems to do a good job.

So now when there is an exception we get the nicer output since this
takes care of all the steps to reraise exceptions in the right spot:

error: 404 Not Found
       GET to /teams
       message: Resource 'account' with ID 'wef' couldn't be found.
       request_id: 26ac03b2-159e-4f0f-809e-84c474fe7b79

But then I realized that you can't actually call get clusters and get
teams at the same time, since you need to know the teams in order to
list their clusters. And this whole time for completion we were making
an extra call to get teams, since get_clusters with no args makes it's
own call to get teams. So I removed that unnecessary call from the
completion. The unhandeled exception problem though is a potential
problem in get_clusters, so it makes sense to use the new Promise
library there I think.

@will will requested a review from abrightwell June 18, 2022 00:58
Copy link
Member

@abrightwell abrightwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completion_spec needs something like

  def get_clusters(teams : Array(Team))
    [Cluster.new("abc", "def", "my cluster", [] of Cluster)]
  end

  def get_clusters(team_id : String)
    [Cluster.new("abc", "def", "my cluster", [] of Cluster)]
  end

For the tests to pass.

@will will force-pushed the promise branch 2 times, most recently from cafe317 to cafe7b9 Compare June 21, 2022 17:10
The first try at manually using spawn and channels worked, but if there
was an exception raised, it'd print a stack trace like this instead of
being handled nicely

    Unhandled exception in spawn:  (CB::Client::Error)
      from src/cb/client.cr:517:5 in 'exec'
      from src/cb/client.cr:484:5 in 'get'
      from src/cb/client.cr:144:5 in 'get_teams'
      from src/cb/completion.cr:126:22 in '->'
      from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:146:11 in 'run'
      from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:98:34 in '->'
    Unhandled exception in spawn:  (CB::Client::Error)
      from src/cb/client.cr:517:5 in 'exec'
      from src/cb/client.cr:484:5 in 'get'
      from src/cb/client.cr:144:5 in 'get_teams'
      from src/cb/client.cr:234:18 in 'get_clusters'
      from src/cb/completion.cr:128:22 in '->'
      from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:146:11 in 'run'
      from /usr/local/Cellar/crystal/1.4.1/src/fiber.cr:98:34 in '->'

The way to get around this would be to have the channel also be able to
take an exception, and resuce exceptions in the spawn block and send
them on the channel, then raise them in the main thread. This is a lot
of boilerplate work. I started writing a thing to do this for us, then
realized it was basically the promise pattern, and that someone else
probably has done that. I was a little hesitant to pull in this library
but it's not that large, and seems to do a good job.

So now when there is an exception we get the nicer output since this
takes care of all the steps to reraise exceptions in the right spot:

    error: 404 Not Found
           GET to /teams
           message: Resource 'account' with ID 'wef' couldn't be found.
           request_id: 26ac03b2-159e-4f0f-809e-84c474fe7b79

But then I realized that you can't actually call get clusters and get
teams at the same time, since you need to know the teams in order to
list their clusters. And this whole time for completion we were making
an extra call to get teams, since get_clusters with no args makes it's
own call to get teams. So I removed that unnecessary call from the
completion. The unhandeled exception problem though is a potential
problem in get_clusters, so it makes sense to use the new Promise
library there I think.
@will will merged commit cafe7b9 into main Jun 21, 2022
@will will deleted the promise branch June 21, 2022 17:18
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.

None yet

2 participants