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.

Sign up for GitHub

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

Jump to bottom

feat: tokio upgrade #729

Merged
merged 29 commits into from
Aug 12, 2021
Merged

feat: tokio upgrade #729

merged 29 commits into from
Aug 12, 2021

Conversation

Mirko-von-Leipzig
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig commented Aug 2, 2021 •
edited
Loading

Quite a large and difficult refactor. There are some issues remaining which require feedback.

  1. There is now a stack-overflow when running in debug mode. This is a bug in warp
  2. Two FIXME todo! I left for format to strings where I don't know what info is valuable.

Test status remained the same for me before and after. Tests can all pass; but the ilp-node tests seem plagued by racey start-up conditions (?) and sporadically fail -- this behaviour was the same before the refactor.

--edit--
Added a fix for (1) -- tests now pass in debug mode. We will want to ensure that other filter handlers are covered by tests, but that should be a new issue.

koivunej
koivunej reviewed Aug 2, 2021
View reviewed changes
crates/ilp-node/Cargo.toml Outdated Show resolved Hide resolved
@koivunej
Copy link
Collaborator

koivunej commented Aug 2, 2021

There is now a stack-overflow when running in debug mode.

How come this didn't manifest itself in test-md build...? Surely it does a lot of filters. Did you add unconditional boxing to the filters and I missed it while doing the commit-per-commit?

@Mirko-von-Leipzig
Copy link
Contributor Author

Mirko-von-Leipzig commented Aug 2, 2021 •
edited
Loading

There is now a stack-overflow when running in debug mode.

How come this didn't manifest itself in test-md build...? Surely it does a lot of filters. Did you add unconditional boxing to the filters and I missed it while doing the commit-per-commit?

I did try inserting .boxed on all the routes/filters I could find. But I never managed to "fix" the issue -- I presumed its because I didn't know where to look in the codebase. So its possible its a different issue; but its unlikely as:

  1. Release build works, Debug does not (as described in the issue)
  2. Using println debugging indicates that its when the warp serve gets called.

I'm not sure about test-md.

@Mirko-von-Leipzig
Copy link
Contributor Author

Mirko-von-Leipzig commented Aug 2, 2021 •
edited
Loading

Force pushed to include fmt fixes

@koivunej koivunej mentioned this pull request Aug 3, 2021
Merged
@Mirko-von-Leipzig Mirko-von-Leipzig force-pushed the feat/tokio_upgrade branch 2 times, most recently from d0e4d29 to ec880fe Compare August 3, 2021 12:34
@koivunej koivunej mentioned this pull request Aug 4, 2021
Open
@Mirko-von-Leipzig
Copy link
Contributor Author

Rebase'd master changes (CI speed improvements)

@Mirko-von-Leipzig
Copy link
Contributor Author

Commit e72bd5c introduces a work-around fix for the warp stack-overflow bug (mentioned in PR description).

Further investigation indicates that no other stack-overflow issues remain (from this bug) -- used test coverage to check all instances of filters are covered or manually inspected if missed.

koivunej
koivunej approved these changes Aug 11, 2021
View reviewed changes
Copy link
Collaborator

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Oki with the stack overflow issues hopefully avoided. I forgot to give approval for this initially, so I guess we'll await until tomorrow before merging.

@Mirko-von-Leipzig
feat: upgrade redis to 0.21.0
a5543f9 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade secrecy to 0.8
0f46280 
Required manually implementing Serialization for `Secret<String>` as
this is purposefully disabled by default.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade bytes to 1.0.1
533aa5f 
Main API changes are `bytes()` to `chunks()` and similar renamings.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade tokio to 1.9.0
91bff9f 
Does not compile, requires changes to many other dependencies as well.

Main API change is `delay_for` to `sleep`.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: add tokio-util for DelayQueue
bd0e27c 
`DelayQueue` was moved from `tokio` to `tokio-util` in preparation
for the 1.0 release of `tokio`.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: add tokio-stream
b4c7235 
This fixes the missing `Stream` adapters for `tokio` related items.

For example, `tokio::sync::Receiver<T>` used to implement `FutureExt`
trait, but it no longer does. Instead we now wrap it using
`tokio-stream` to provide the same extension.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade warp to 0.3.1
f10ec56 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade hyper to 0.14.11
abc4b42 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade futures-retry to 0.6.0
8780f21 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upade Cargo.lock
f16fcd3 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade reqwest to 0.11.4
434846c 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade socket2 to 0.4.0
548557d 
Note that version "0.3.15" was yanked.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
fix: redis test Secret<String> is not serializable
2dcf442 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
fix: mut not needed
0e7d855 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
fix: tests require tokio_stream
b273460 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: tokio-tungstenite upgrade
c65af22 
Also includes removing the base tungstenite dependency. Since
tokio-tungstenite re-exports it.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: upgrade socket2 to 0.4.0
c79bad3 
Previous version was yanked.

Changes are
- `Domain::ipv4()` -> `Domain::IPV4`
- `Type::stream()` -> `Type::STREAM`
- `socket.into_tcp_listener()` -> `TcpListener::from(socket)`

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: updated stream-cancel to 0.8.1
9746923 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: updated futures-retry to 0.6.0 (missed some)
6d3e8cb 
futures-retry API changes:
- `#attempts` is now tracked by the library
- retry output changed: `Result<output, err>` to
`Result<(output, attempts), (err, attempts)>`

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
fix: requests retry count is off by one
28ece7d 
Maximum retries were being passed in to the RequestErrorHandler as is.
However in RequestErrorHandler it is defined as maximum attempts.

i.e. `N > max` versus `N == max`

In the future-retries upgrade the check for this was corrected to match
the attempts naming, but this broke the actual usage which assumed it
was retries.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: update cargo.lock
ee5a5b5 
Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
fix: warp filter stack-overflow
0efd1f4 
Fix for stack-overflow in debug mode, caused by too many warp filters.
Issue link: seanmonstar/warp#811

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
ci: remove net2 audit check (not required)
96a674b 
Was a tokio 0.2 dependency, no longer needed.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
test: change error log to eprintln!
ec55027 
The error handler is rarely / never configured for tests, and this error
was therefore not visible.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: updated yup-oauth2 to 5.1.0
c404516 
Part of upgrading tokio from v0.2 to v1.*.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
ci: remove hyper rustsec audit ignore
27e7de7 
Patched for versions >=0.14.10, and we now use 0.14.11.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
ci: remove sized-chunk audit ignore
7fb5bc8 
Patched in versions >=0.6.3, we use 0.6.4.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: update base64 to 0.13.0
4882157 
This now matches the version used by our dependencies, so that we
only depend on one version overall.

Signed-off-by: Mirko von Leipzig <[email protected]>
@Mirko-von-Leipzig
feat: downgrade tokio-tungstenite to 0.13.0
6fa10a7 
This matches the version used by warp, and means we only depend on
a single version. The downgrade doesn't remove any functionality we
rely on.

Signed-off-by: Mirko von Leipzig <[email protected]>
@koivunej
Copy link
Collaborator

Allrighty, this is ready to merge. This a big step forwards, thanks a lot!

@koivunej koivunej merged commit 9013c94 into interledger:master Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koivunej koivunej koivunej approved these changes

Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Mirko-von-Leipzig @koivunej