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

Changes from 1 commit
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
4 changes: 2 additions & 2 deletions libp2p/protocols/protocol.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ type
conn: Connection,
proto: string): Future[void] {.async.}

LPProtoHandler2* = proc (
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): Future[void] {.async: (raises: [CancelledError]).}
proto: string): InternalRaisesFuture[void, E]

LPProtocol* = ref object of RootObj
codecs*: seq[string]
Expand Down
Loading