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

add support for setting protocol handlers with {.raises.} annotation #1064

Merged
merged 8 commits into from
Mar 28, 2024
Merged

add support for setting protocol handlers with {.raises.} annotation #1064

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
14fee3b
add support for setting protocol handlers with `{.raises.}` annotation
etan-status Mar 11, 2024
417922c
example also needs a change, not sure if acceptable
etan-status Mar 11, 2024
302359b
allow any explicit `raises` for `LPProtoHandler2`
etan-status Mar 18, 2024
69f18ec
add comment for `InternalRaisesFuture` Nim 2.0 bug
etan-status Mar 21, 2024
843fb45
rename `LPProtoHandler2` -> `LPProtocolHandler`
etan-status Mar 21, 2024
967c40c
add documentation for `LPProtocolHandler`
etan-status Mar 21, 2024
71dbcef
Merge branch 'unstable' into dev/etan/ex-handlersupport
etan-status Mar 25, 2024
3af3aa5
avoid typedef for `LPProtocolHandler`
etan-status Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/helloworld.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ proc new(T: typedesc[TestProto]): T =
# We must close the connections ourselves when we're done with it
await conn.close()

return T(codecs: @[TestCodec], handler: handle)
return T.new(codecs = @[TestCodec], handler = handle)

##
# Helper to create a switch/node
Expand Down
4 changes: 2 additions & 2 deletions libp2p/multistream.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Nim-LibP2P
# Copyright (c) 2023 Status Research & Development GmbH
# Copyright (c) 2023-2024 Status Research & Development GmbH
# Licensed under either of
# * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE))
# * MIT license ([LICENSE-MIT](LICENSE-MIT))
Expand Down Expand Up @@ -230,7 +230,7 @@ proc addHandler*(m: MultistreamSelect,

proc addHandler*(m: MultistreamSelect,
codec: string,
handler: LPProtoHandler,
handler: LPProtoHandler|LPProtoHandler2,
matcher: Matcher = nil) =
## helper to allow registering pure handlers
trace "registering proto handler", proto = codec
Expand Down
50 changes: 38 additions & 12 deletions libp2p/protocols/protocol.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Nim-LibP2P
# Copyright (c) 2023 Status Research & Development GmbH
# Copyright (c) 2023-2024 Status Research & Development GmbH
# Licensed under either of
# * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE))
# * MIT license ([LICENSE-MIT](LICENSE-MIT))
Expand All @@ -19,14 +19,16 @@ const

type
LPProtoHandler* = proc (
conn: Connection,
proto: string):
Future[void]
{.gcsafe, raises: [].}
conn: Connection,
proto: string): Future[void] {.async.}

LPProtoHandler2*[E] = proc (
Copy link
Contributor

Choose a reason for hiding this comment

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

does lp2[E] = proc ...: Future[void].Raising(E) work?

Copy link
Contributor Author

@etan-status etan-status Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the handler= implementation worked for me.

I tested it by changing protocols/connectivity/relay/client.nim to

proc handleStream(conn: Connection, proto: string) {.async: (raises: [CancelledError]).} =
  ...
cl.handler = handleStream

and NIMFLAGS="--mm:refc" nimble test still worked fine (refc because on orc it segfaults in metrics unrelated).

Copy link
Contributor

Choose a reason for hiding this comment

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

no, I mean you don't need to expose InternalRaisesFuture here (we can avoid it here too probably: #1050 (comment)):

import chronos
type T[E] = Future[void].Raising(E)
proc x() {.async: (raises: [ValueError]).} = discard
proc f[E](handler: proc(): T[E]) = debugEcho type(handler)

f(x)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the proc takes parameters and is passed around, hose parts would have to be continuously repeated. putting the [E] directly on the proc doesn't work, only the result type can be done that way.

Exposing InternalRaisesFuture in a single place seems like the lesser evil to me, it's trivial to adjust if necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO adding any *2 to the codebase isn't a good software engineering practice as it isn't sufficiently descriptive. Also, why is an Ineternal* type from another project referenced here?

Copy link
Contributor

@arnetheduck arnetheduck Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

have never seen such naming used in mainstream libraries.

The most used and popular api on earth does this, more or less: https://learn.microsoft.com/en-us/windows/win32/api/winver/nf-winver-getfileversioninfoexa (though they call it Ex instead of 2)

I understand your concern, but sometimes, there just isn't any good naming solution and indeed, a comment might be the best option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They also have Ex2: https://learn.microsoft.com/en-us/windows/win32/api/vds/nf-vds-ivdsvolumemf3-formatex2

Copy link
Contributor

@arnetheduck arnetheduck Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

with a cherry on top for 3 in the interface name ;) in fact, com interfaces / all of .NET often versions itself this way

Copy link
Contributor

Choose a reason for hiding this comment

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

I knew I'd regret the comment, but never worked with MS tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

So .. thinking a bit more about this, the answer is actually simple: we can remove LPProtocolHandler/LPProtoHandler2 altogether and just use Future[...].Raising(E) explicitly in the wrapper, which ensures user code is not polluted with a name that we don't necessarily want to support.

This makes sense also because there as limitations to how you can use aliases together with `` ie var f: LPProtocolHandler[[ValueError]] doesn't work because of macro instantiation order issues.

conn: Connection,
proto: string): InternalRaisesFuture[void, E]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LPProtoHandler2*[E] = proc (
conn: Connection,
proto: string): InternalRaisesFuture[void, E]
LPProtoHandler2*[E] = proc (
conn: Connection,
proto: string): Future[void].Raising(E)

this does not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, not on the proc, only on the result


LPProtocol* = ref object of RootObj
codecs*: seq[string]
handler*: LPProtoHandler ## this handler gets invoked by the protocol negotiator
handlerImpl: LPProtoHandler ## invoked by the protocol negotiator
Copy link
Contributor

Choose a reason for hiding this comment

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

this rename breaks LPProtocol(handler: ...) - not sure it's worth it: new code can use LPProtocol.new to get the covariance handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I mentioned above. It has to be renamed otherwise handler= doesn't take precedence, and handler= is used a lot, while using LPProtocol(...) over LPProtocol.new was only used in 1 example and 1 test, where it seems to be not intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

breaks nimbus-eth2 though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 liner to fix

started*: bool
maxIncomingStreams: Opt[int]

Expand All @@ -41,23 +43,47 @@ proc `maxIncomingStreams=`*(p: LPProtocol, val: int) =
p.maxIncomingStreams = Opt.some(val)

func codec*(p: LPProtocol): string =
assert(p.codecs.len > 0, "Codecs sequence was empty!")
doAssert(p.codecs.len > 0, "Codecs sequence was empty!")
p.codecs[0]

func `codec=`*(p: LPProtocol, codec: string) =
# always insert as first codec
# if we use this abstraction
p.codecs.insert(codec, 0)

template `handler`*(p: LPProtocol): LPProtoHandler =
p.handlerImpl

template `handler`*(
p: LPProtocol, conn: Connection, proto: string): Future[void] =
p.handlerImpl(conn, proto)

func `handler=`*(p: LPProtocol, handler: LPProtoHandler) =
p.handlerImpl = handler

func `handler=`*(p: LPProtocol, handler: LPProtoHandler2) =
proc wrap(conn: Connection, proto: string): Future[void] {.async.} =
await handler(conn, proto)
p.handlerImpl = wrap

proc new*(
T: type LPProtocol,
codecs: seq[string],
handler: LPProtoHandler,
maxIncomingStreams: Opt[int] | int = Opt.none(int)): T =
T: type LPProtocol,
codecs: seq[string],
handler: LPProtoHandler,
maxIncomingStreams: Opt[int] | int = Opt.none(int)): T =
T(
codecs: codecs,
handler: handler,
handlerImpl: handler,
maxIncomingStreams:
when maxIncomingStreams is int: Opt.some(maxIncomingStreams)
else: maxIncomingStreams
)

proc new*(
T: type LPProtocol,
codecs: seq[string],
handler: LPProtoHandler2,
maxIncomingStreams: Opt[int] | int = Opt.none(int)): T =
proc wrap(conn: Connection, proto: string): Future[void] {.async.} =
await handler(conn, proto)
T.new(codec, wrap, maxIncomingStreams)
5 changes: 2 additions & 3 deletions tests/testtortransport.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{.used.}

# Nim-Libp2p
# Copyright (c) 2023 Status Research & Development GmbH
# Copyright (c) 2023-2024 Status Research & Development GmbH
# Licensed under either of
# * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE))
# * MIT license ([LICENSE-MIT](LICENSE-MIT))
Expand Down Expand Up @@ -88,7 +88,6 @@ suite "Tor transport":

# every incoming connections will be in handled in this closure
proc handle(conn: Connection, proto: string) {.async.} =

var resp: array[6, byte]
await conn.readExactly(addr resp, 6)
check string.fromBytes(resp) == "client"
Expand All @@ -97,7 +96,7 @@ suite "Tor transport":
# We must close the connections ourselves when we're done with it
await conn.close()

return T(codecs: @[TestCodec], handler: handle)
return T.new(codecs = @[TestCodec], handler = handle)

let rng = newRng()

Expand Down
Loading