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

Refactor towards more idiomatic async code #253

Merged
merged 3 commits into from Dec 6, 2019
Merged

Refactor towards more idiomatic async code #253

merged 3 commits into from Dec 6, 2019

Conversation

@thomaseizinger
Copy link
Member

thomaseizinger commented Dec 4, 2019

This refactoring addresses several pain points of the current codebase:

  1. We used to write to the env file from several places. This made it hard to understand, what was actually written to the file. It was also error prone because there were several concurrent tasks doing it.
  2. Many APIs were stringly-typed. This made it hard to understand, what information is actually passed around.
  3. The codebase was still using old Futures in many places. Using new futures allows us to use async/await which makes the code a lot shorter.

Fixes #37.
Fixes #141.
Fixes #101. (Other PRs did work here as-well but with this one, things are actually fairly nice.)
Fixes #40.

This refactoring addresses several pain points of the current
codebase:

1. We used to write to the env file from several places. This made
it hard to understand, what was actually written to the file. It
was also error prone because there were several concurrent tasks
doing it.
2. Many APIs were stringly-typed. This made it hard to understand,
what information is actually passed around.
3. The codebase was still using old Futures in many places. Using
new futures allows us to use async/await which makes the code a
lot shorter.
@thomaseizinger thomaseizinger requested review from D4nte, da-kami and luckysori Dec 4, 2019
@thomaseizinger thomaseizinger self-assigned this Dec 4, 2019
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 4, 2019

Are you sure the changelog does not need updating?

Now that the `new` function is async, we should not use blocking
calls to std. Since we will eventually transition to async-std
anyway, we can start depending on it here.
@thomaseizinger

This comment has been minimized.

Copy link
Member Author

thomaseizinger commented Dec 5, 2019

I noticed that it is not much work to remove the last Futures-0.1 bits, so I am doing that now :)

@thomaseizinger

This comment has been minimized.

Copy link
Member Author

thomaseizinger commented Dec 5, 2019

Will re-open once ready to review.

@thomaseizinger

This comment has been minimized.

Copy link
Member Author

thomaseizinger commented Dec 5, 2019

Unfortunately, we cannot yet fully migrate to async_std because shiplift depends on tokio 0.1 internally.

FYI @D4nte

LogMessage("Flushed wallet.dat"),
)
.await?;

This comment has been minimized.

Copy link
@tcharding

tcharding Dec 6, 2019

Contributor

This has a race condition. When free_local_port() returns the socket is closed. If another process opens a socket it may be on the port returned by free_local_port() and then docker will not be able to successfully start. I cannot think of a way to solve this without burdening the user to configure the port number and the race condition is probably not a big deal here, might be worth noting it in a comment.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Dec 6, 2019

Author Member

Well spotted!

Apparently, there is logic in the kernel(s) that still makes this work. To properly fix it, we would have to delegate the port assignment to docker by passing 0 and then read using docker inspect, which port we got (that is actually what we do in testcontainers :) )

@da-kami
da-kami approved these changes Dec 6, 2019
Copy link
Contributor

da-kami left a comment

I ran all the examples against an environment spun up with this branch multiple times. Worked without problems
Great work @thomaseizinger

@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 6, 2019

bors r+

bors bot added a commit that referenced this pull request Dec 6, 2019
Merge #253
253: Refactor towards more idiomatic async code r=mergify[bot] a=thomaseizinger

This refactoring addresses several pain points of the current codebase:

1. We used to write to the env file from several places. This made it hard to understand, what was actually written to the file. It was also error prone because there were several concurrent tasks doing it.
2. Many APIs were stringly-typed. This made it hard to understand, what information is actually passed around.
3. The codebase was still using old Futures in many places. Using new futures allows us to use async/await which makes the code a lot shorter.

Fixes #37. 
Fixes #141. 
Fixes #101. (Other PRs did work here as-well but with this one, things are actually fairly nice.)
Fixes #40.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Dec 6, 2019

Build succeeded

  • build (windows)
  • e2e_test
  • build (macos)
  • build (ubuntu)
  • static_analysis
@bors bors bot merged commit 05bf1aa into master Dec 6, 2019
9 checks passed
9 checks passed
static_analysis
Details
build (macos)
Details
build (windows)
Details
build (ubuntu)
Details
npm_build
Details
e2e_test
Details
Summary 3 rules match
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@mergify mergify bot deleted the awesome-refactoring branch Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.