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 2 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* = proc (
Copy link
Contributor

@arnetheduck arnetheduck 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.

hm..

not that I necessarily disagree with this change, but this becomes an odd case which should have been solved by covariance - ie technically, because we allow any exception to be raised, we should not constrain here to [CancelledError] (raises: [ValueError] would also be "correct")..

I wonder if there's a way to solve this with some generics, ie create a new that takes an new[E](handler: proc(): InternalRaisesFuture[void, E] - if that's possible, it's probably something we should formalize in chronos somehow, to allow expressing it without referencing Internal..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, it would still involve going through the new or handler= instead of constructing the object manually. Agree that making it more generic would also serve the role of allowing {.async: raises.} annotation, but for now would have to go with Internal. I guess that's alright to get started.

conn: Connection,
proto: string): Future[void] {.async: (raises: [CancelledError]).}

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