Add windows Ipc Client implementation #7187

AbnerZheng · 2024-03-17T17:20:55Z

AbnerZheng · 2024-03-17T17:54:53Z

@mattsse PTAL when you available and let me know if I'm heading in the right direction. Thanks!

mattsse

awesome, one suggestion re structure

mattsse · 2024-03-19T11:54:17Z

crates/rpc/ipc/src/client.rs

 pub struct Receiver {
-    inner: FramedRead<tokio::net::unix::OwnedReadHalf, StreamCodec>,
+    #[cfg(unix)]
+    inner: FramedRead<OwnedReadHalf, StreamCodec>,
+    #[cfg(windows)]
+    inner: FramedRead<Arc<NamedPipeClient>, StreamCodec>,


this is great,

what I would like to do instead is:
convert client into a module with
unix.rs
win.rs

and duplicate the types then client/mod.rs reexports based on windows or UNIX

onbjerg

thank you for taking this on 👑

crates/rpc/ipc/src/client/unix.rs

AbnerZheng · 2024-03-19T16:08:50Z

@mattsse @onbjerg
There raise some issues when I tried to use cross to build and test.

> cross test --target x86_64-pc-windows-gnu -p reth-ipc

error[E0432]: unresolved import `winapi::shared::winerror`
 --> /Users/abnerzheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/parity-tokio-ipc-0.9.0/src/win.rs:1:21
  |
1 | use winapi::shared::winerror::{ERROR_PIPE_BUSY, ERROR_SUCCESS};
  |                     ^^^^^^^^ could not find `winerror` in `shared`

Looks like parity-tokio-ipc can't be compiled in windows anymore and out of maintainence. See paritytech/parity-tokio-ipc#39 and paritytech/parity-tokio-ipc#36

The server side implementation relay on parity-tokio-ipc.

onbjerg · 2024-03-19T16:14:48Z

I think we only use this for the Endpoint type, we should be able to just roll our own, IIRC we had similar issues in Foundry and ditched this crate? cc @mattsse

mattsse · 2024-03-19T17:35:56Z

yeah this is indeed unmaintained and we should actually switch

Is the cross build issue a problem?
I think we can live with this for now?

AbnerZheng · 2024-03-19T17:54:06Z

Is the cross build issue a problem?

I think if we try to build ipc crate in windows machine, then it will issue the error.

I think we can live with this for now?

Yes, but then we have to guard the 'server' mod with #[cfg(unix)] as well, because it won't compile in windows system.

mattsse · 2024-03-19T18:22:23Z

I think we should transition to:

https://github.com/kotauskas/interprocess

AbnerZheng · 2024-03-19T18:30:27Z

I think we should transition to:

https://github.com/kotauskas/interprocess

No problem, I will continue the transition in tomorrow.

onbjerg · 2024-04-18T11:07:52Z

Hey @AbnerZheng! Are you blocked on anything here? Let me know 😊

AbnerZheng · 2024-04-18T12:04:28Z

Let me know 😊

Would take it another try this weekend and will ping you if there are any problem.

AbnerZheng · 2024-04-26T09:58:26Z

@onbjerg I have switched to use interprocess v1.2.1, which would unblock this PR.

AbnerZheng · 2024-04-26T09:59:38Z

deny.toml

 # See https://spdx.org/licenses/ for list of possible licenses
 # [possible values: any SPDX 3.7 short identifier (+ optional exception)].
 allow = [
+    "CC0-1.0",


@onbjerg @mattsse I have to allowed this license because interprocess use a library which is under it.

can you add it under exceptions like the other CC0-1.0 deps we have?

AbnerZheng · 2024-04-26T10:03:08Z

crates/rpc/ipc/src/server/mod.rs

-        on_ready.send(Ok(())).ok();
-
-        let mut incoming = Monitored::new(incoming, &stop_handle);
+        let mut connections = FuturesUnordered::new();


jsonrpsee has switch to use FuturesUnordered in PR paritytech/jsonrpsee#1062, which address some issues when upgrading tokio. The PR also removed the need of Incoming, Monitored, which unblock this PR.

onbjerg

prelim look this looks fine to me, ptal @mattsse

please move the license exception into where we have our other CC0 deps

mattsse

this is great, ty

but futuresunordered has to go

mattsse · 2024-04-26T13:04:03Z

crates/rpc/ipc/src/server/mod.rs

+        // FuturesUnordered won't poll anything until this line but because the
+        // tasks are spawned (so that they can progress independently)
+        // then this just makes sure that all tasks are completed before
+        // returning from this function.
+        while connections.next().await.is_some() {}


this is unreachable and results in unbounded mem

please check jsonrpsee server loop on main

https://github.com/paritytech/jsonrpsee/blob/master/server/src/server.rs

Yeah, but would like merge it first, and fire a new PR later today.

okay sg, could you please open an issue for this then?

#7916
would fire a PR today.

crates/rpc/ipc/src/server/mod.rs

mattsse

lgtm,
needs followup re fut unordered

AbnerZheng added 3 commits March 18, 2024 01:20

Add windows Ipc Client implementation

f6113e8

Merge branch 'paradigmxyz:main' into issue-6526

67a08fe

clippy

cf2a2d6

enable test in windows

ab46935

mattsse requested changes Mar 19, 2024

View reviewed changes

mattsse added C-enhancement New feature or request A-rpc Related to the RPC implementation labels Mar 19, 2024

refactor

da75264

AbnerZheng marked this pull request as ready for review March 19, 2024 15:52

AbnerZheng requested review from Rjected and gakonst as code owners March 19, 2024 15:52

onbjerg requested changes Mar 19, 2024

View reviewed changes

crates/rpc/ipc/src/client/unix.rs Outdated Show resolved Hide resolved

clippy

6f2e639

wip

8a3192e

AbnerZheng closed this Mar 25, 2024

AbnerZheng deleted the issue-6526 branch March 25, 2024 18:33

AbnerZheng restored the issue-6526 branch March 26, 2024 02:20

AbnerZheng reopened this Mar 26, 2024

github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Apr 17, 2024

onbjerg mentioned this pull request Apr 18, 2024

fix: don't import ipc client on windows #7712

Merged

onbjerg removed the S-stale This issue/PR is stale and will close with no further activity label Apr 18, 2024

onbjerg added the S-blocked This cannot more forward until something else changes label Apr 25, 2024

AbnerZheng added 4 commits April 26, 2024 15:13

Merge branch 'main' into issue-6526

6939aea

use interprocess 1.2.1 and remove FutureDriver

be008d9

fix lint

e672755

allow CC0-1.0 license

d54da13

AbnerZheng commented Apr 26, 2024

View reviewed changes

AbnerZheng added 2 commits April 26, 2024 18:10

apply suggestions from review

d1d2e80

apply suggestions from review

c00bea4

AbnerZheng requested review from mattsse and onbjerg April 26, 2024 10:15

AbnerZheng added 4 commits April 26, 2024 18:17

fix lint

968349f

fix lint

5fccb86

fix cargo doc check

28d88e9

apply suggestions from review

1bd9884

onbjerg removed the S-blocked This cannot more forward until something else changes label Apr 26, 2024

onbjerg approved these changes Apr 26, 2024

View reviewed changes

apply suggestions from review

c7f8bfb

mattsse requested changes Apr 26, 2024

View reviewed changes

apply suggestions from review

67090da

mattsse approved these changes Apr 26, 2024

View reviewed changes

AbnerZheng mentioned this pull request Apr 26, 2024

Remove FuturesUnordered in ipc #7916

Closed

reformat

921f39b

mattsse added this pull request to the merge queue Apr 26, 2024

Merged via the queue into paradigmxyz:main with commit b6b2cf8 Apr 26, 2024

AbnerZheng deleted the issue-6526 branch April 26, 2024 13:47

AbnerZheng mentioned this pull request Apr 26, 2024

feat: replace low level IPC with interprocess #7922

Merged

Add windows Ipc Client implementation #7187

Add windows Ipc Client implementation #7187

Uh oh!

Conversation

AbnerZheng commented Mar 17, 2024

Uh oh!

AbnerZheng commented Mar 17, 2024

Uh oh!

mattsse left a comment

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!

onbjerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AbnerZheng commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onbjerg commented Mar 19, 2024

Uh oh!

mattsse commented Mar 19, 2024

Uh oh!

AbnerZheng commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse commented Mar 19, 2024

Uh oh!

AbnerZheng commented Mar 19, 2024

Uh oh!

onbjerg commented Apr 18, 2024

Uh oh!

AbnerZheng commented Apr 18, 2024

Uh oh!

AbnerZheng commented Apr 26, 2024

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!

onbjerg left a comment • 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!

mattsse left a comment

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!

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AbnerZheng commented Mar 19, 2024 •

edited

Loading

AbnerZheng commented Mar 19, 2024 •

edited

Loading

onbjerg left a comment •

edited

Loading