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

etan-status · 2024-03-11T14:22:55Z

All of the internal protocol handlers are restricted to {.raises.} of [CancelledError]. However, the LPProtoHandler type is part of the public interface, and example / test code raises more errors than just [CancelledError]. The caller is aware of that and CatchableError is caught.

To allow annotating the internal handlers with the proper {.raises.} annotation, support for an extra LPProtoHandler2 is added. This is backward compatible as the old LPProtoHandler is still allowed. Examples still compile fine. There is one test that needs a slight adjustment as it accesses the internal handler field directly. That field needs to be renamed so that the template is used instead.

Eventually, LPProtoHandler may be phased out, with appropriate notices to users who define custom handlers and the migration requiring errors to be handled inside the handler instead of raising them. At this time, such a deprecation is not yet applied, especially while the internal logic is still relying on the previous handler flow.

All of the internal protocol handlers are restricted to `{.raises.}` of `[CancelledError]`. However, the `LPProtoHandler` type is part of the public interface, and example / test code raises more errors than just `[CancelledError]`. The caller is aware of that and `CatchableError` is caught. To allow annotating the internal handlers with the proper `{.raises`.} annotation, support for an extra `LPProtoHandler2` is added. This is backward compatible as the old `LPProtoHandler` is still allowed. Examples still compile fine. There is one test that needs a slight adjustment as it accesses the internal `handler` field directly. That field needs to be renamed so that the `template` is used instead. Eventually, `LPProtoHandler` may be phased out, with appropriate notices to users who define custom handlers and the migration requiring errors to be handled inside the handler instead of raising them. At this time, such a deprecation is not yet applied, especially while the internal logic is still relying on the previous handler flow.

codecov · 2024-03-11T14:47:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.56%. Comparing base (08a48fa) to head (417922c).

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #1064   +/-   ##
=========================================
  Coverage     82.56%   82.56%           
=========================================
  Files            91       91           
  Lines         15814    15816    +2     
=========================================
+ Hits          13057    13059    +2     
  Misses         2757     2757

Files	Coverage Δ
libp2p/multistream.nim	`93.71% <ø> (ø)`
libp2p/protocols/protocol.nim	`100.00% <100.00%> (ø)`

arnetheduck · 2024-03-18T08:26:35Z

libp2p/protocols/protocol.nim

+ conn: Connection,
+ proto: string): Future[void] {.async.}
+
+ LPProtoHandler2* = proc (


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

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.

arnetheduck · 2024-03-18T15:45:00Z

libp2p/protocols/protocol.nim

@@ -22,9 +22,9 @@ type
 conn: Connection,
 proto: string): Future[void] {.async.}

- LPProtoHandler2* = proc (
+ LPProtoHandler2*[E] = proc (


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

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).

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)

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

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?

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.

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

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

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

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.

arnetheduck · 2024-03-21T12:17:18Z

libp2p/protocols/protocol.nim

+ LPProtoHandler2*[E] = proc (
+ conn: Connection,
+ proto: string): InternalRaisesFuture[void, E]


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?

nope, not on the proc, only on the result

arnetheduck · 2024-03-21T14:24:32Z

libp2p/protocols/protocol.nim


 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


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

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.

breaks nimbus-eth2 though

1 liner to fix

Avoid potenial issue with vacp2p/nim-libp2p#1064 (comment) in a future dependency bump.

diegomrsantos · 2024-03-21T15:32:18Z

libp2p/protocols/protocol.nim

+ conn: Connection,
+ proto: string): Future[void] {.async.}
+
+ # Callbacks that are annotated with `{.async: (raises).}` explicitly


looks great, thanks.

Avoid potenial issue with vacp2p/nim-libp2p#1064 (comment) in a future dependency bump.

example also needs a change, not sure if acceptable

417922c

arnetheduck reviewed Mar 18, 2024

View reviewed changes

allow any explicit raises for LPProtoHandler2

302359b

arnetheduck reviewed Mar 18, 2024

View reviewed changes

arnetheduck reviewed Mar 21, 2024

View reviewed changes

add comment for InternalRaisesFuture Nim 2.0 bug

69f18ec

etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Mar 21, 2024

use LPProtocol.new instead of LPProtocol()

d249e75

Avoid potenial issue with vacp2p/nim-libp2p#1064 (comment) in a future dependency bump.

etan-status mentioned this pull request Mar 21, 2024

use LPProtocol.new instead of LPProtocol() status-im/nimbus-eth2#6117

Merged

etan-status added 2 commits March 21, 2024 15:39

rename LPProtoHandler2 -> LPProtocolHandler

843fb45

add documentation for LPProtocolHandler

967c40c

diegomrsantos reviewed Mar 21, 2024

View reviewed changes

etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Mar 21, 2024

use LPProtocol.new instead of LPProtocol() (#6117)

9256db2

Avoid potenial issue with vacp2p/nim-libp2p#1064 (comment) in a future dependency bump.

etan-status added 2 commits March 25, 2024 18:08

Merge branch 'unstable' into dev/etan/ex-handlersupport

71dbcef

avoid typedef for LPProtocolHandler

3af3aa5

arnetheduck approved these changes Mar 28, 2024

View reviewed changes

arnetheduck merged commit 03f67d3 into unstable Mar 28, 2024
9 checks passed

arnetheduck deleted the dev/etan/ex-handlersupport branch March 28, 2024 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

etan-status commented Mar 11, 2024

codecov bot commented Mar 11, 2024 •

edited

arnetheduck Mar 18, 2024 •

edited

etan-status Mar 18, 2024

arnetheduck Mar 18, 2024

etan-status Mar 18, 2024 •

edited

arnetheduck Mar 18, 2024

etan-status Mar 21, 2024

diegomrsantos Mar 21, 2024

arnetheduck Mar 21, 2024 •

edited

etan-status Mar 21, 2024

arnetheduck Mar 21, 2024 •

edited

diegomrsantos Mar 21, 2024

arnetheduck Mar 25, 2024

arnetheduck Mar 21, 2024

etan-status Mar 21, 2024

arnetheduck Mar 21, 2024

etan-status Mar 21, 2024

arnetheduck Mar 21, 2024

etan-status Mar 21, 2024

diegomrsantos Mar 21, 2024

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

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

Conversation

etan-status commented Mar 11, 2024

codecov bot commented Mar 11, 2024 • edited

Codecov Report

arnetheduck Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etan-status Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

codecov bot commented Mar 11, 2024 •

edited

arnetheduck Mar 18, 2024 •

edited

etan-status Mar 18, 2024 •

edited

arnetheduck Mar 21, 2024 •

edited

arnetheduck Mar 21, 2024 •

edited