Add spec for labeled-replies. #162

Merged
merged 19 commits into from Jan 7, 2017

Conversation

Projects
None yet
@DarthGandalf
Member

DarthGandalf commented Jul 12, 2015

Fix #53

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Jul 12, 2015

Member

Topic for bikeshedding: label vs id vs [your choice]

Member

DarthGandalf commented Jul 12, 2015

Topic for bikeshedding: label vs id vs [your choice]

@SaberUK

This comment has been minimized.

Show comment
Hide comment
@SaberUK

SaberUK Jul 12, 2015

Contributor

👍 for id.

Contributor

SaberUK commented Jul 12, 2015

👍 for id.

@dequis

This comment has been minimized.

Show comment
Hide comment
@dequis

dequis Jul 12, 2015

Contributor

Did someone say bikeshedding? I looooove bikeshedding!

👍 for id because it's shorter.

Contributor

dequis commented Jul 12, 2015

Did someone say bikeshedding? I looooove bikeshedding!

👍 for id because it's shorter.

@kythyria

This comment has been minimized.

Show comment
Hide comment
@kythyria

kythyria Jul 12, 2015

Contributor

👍 for id also. It's shorter and still conveys the right concept.

Contributor

kythyria commented Jul 12, 2015

👍 for id also. It's shorter and still conveys the right concept.

@grawity

This comment has been minimized.

Show comment
Hide comment
@grawity

grawity Jul 12, 2015

Contributor

"id" (or "tag") would be better, both because it's shorter, and because I haven't seen any other protocol uses the term "label" for that, so it'd be confusing – it's almost always either a (transaction) ID or a tag. ("Cookie" is also used, but can be easily confused with session-state cookies. And in IRC "tag" would of course be confused with the @foo= tags themselves. So that keeps only id.)

Contributor

grawity commented Jul 12, 2015

"id" (or "tag") would be better, both because it's shorter, and because I haven't seen any other protocol uses the term "label" for that, so it'd be confusing – it's almost always either a (transaction) ID or a tag. ("Cookie" is also used, but can be easily confused with session-state cookies. And in IRC "tag" would of course be confused with the @foo= tags themselves. So that keeps only id.)

core/labeled-replies-3.3.md
+If response contains more than one message, `batch` can be used to group them.
+In that case start of the batch MUST be tagged with the label, and the batch type MUST be `labeled-response`.
+
+If the message doesn't require any response, an empty batch MUST be sent.

This comment has been minimized.

@dequis

dequis Jul 12, 2015

Contributor

What does this mean? Is it something like, if echo-message isn't enabled, labeling privmsgs would result in something like this?

Client: @label=somelabel PRIVMSG #channel :Hello!
Server: @label=somelabel :irc.example.com BATCH +somebatch
Server: :irc.example.com BATCH -somebatch
@dequis

dequis Jul 12, 2015

Contributor

What does this mean? Is it something like, if echo-message isn't enabled, labeling privmsgs would result in something like this?

Client: @label=somelabel PRIVMSG #channel :Hello!
Server: @label=somelabel :irc.example.com BATCH +somebatch
Server: :irc.example.com BATCH -somebatch

This comment has been minimized.

@DarthGandalf

DarthGandalf Jul 13, 2015

Member

Yes. Alternatively, I can add a new verb instead:

Server: @label=somelabel :irc.example.com EMPTY
@DarthGandalf

DarthGandalf Jul 13, 2015

Member

Yes. Alternatively, I can add a new verb instead:

Server: @label=somelabel :irc.example.com EMPTY

This comment has been minimized.

@kerio92

kerio92 Jul 17, 2015

What's stopping a compliant ircd from answering with EMPTY or an empty BATCH to a labeled PRIVMSG and then sending the echoed PRIVMSG that comes from echo-message? Shouldn't the spec stress the fact that if a message does, in fact, require a response, then such response MUST be labeled?

@kerio92

kerio92 Jul 17, 2015

What's stopping a compliant ircd from answering with EMPTY or an empty BATCH to a labeled PRIVMSG and then sending the echoed PRIVMSG that comes from echo-message? Shouldn't the spec stress the fact that if a message does, in fact, require a response, then such response MUST be labeled?

core/labeled-replies-3.3.md
+ email: "alexey-irc@asokolov.org"
+---
+Name of this capability is `labeled-replies`.
+If client requests `labeled-replies`, it MUST also request `batch` capability.

This comment has been minimized.

@SaberUK

SaberUK Jul 13, 2015

Contributor

Might be a good idea to have the server NAK/DEL the cap if batch is not requested/goes away.

@SaberUK

SaberUK Jul 13, 2015

Contributor

Might be a good idea to have the server NAK/DEL the cap if batch is not requested/goes away.

+ Server: @label=mGhe5V7RTV :irc.example.com BATCH +NMzYSq45x labeled-response
+ Server: @batch=NMzYSq45x :irc.example.com 311 client nick ~ident host * :Name
+ ...
+ Server: @batch=NMzYSq45x :irc.example.com 318 client nick :End of /WHOIS list.

This comment has been minimized.

@SaberUK

SaberUK Jul 13, 2015

Contributor

It might be a nice idea to have labelled replies deprecate/replace the "end of xxx" numerics.

@SaberUK

SaberUK Jul 13, 2015

Contributor

It might be a nice idea to have labelled replies deprecate/replace the "end of xxx" numerics.

This comment has been minimized.

@grawity

grawity Jul 13, 2015

Contributor

Labelled replies can't do that, since you'd expect the entire series to have the same label after all.

Batched replies, on the other hand, could.

@grawity

grawity Jul 13, 2015

Contributor

Labelled replies can't do that, since you'd expect the entire series to have the same label after all.

Batched replies, on the other hand, could.

This comment has been minimized.

@DarthGandalf

DarthGandalf Jul 13, 2015

Member

If both caps are enabled, server would need to send nested batches, to be able to provide both batch types...

Client: @label=mGhe5V7RTV WHOIS nick
Server: @label=mGhe5V7RTV :irc.example.com BATCH +1 labeled-response
Server: @batch=1 :irc.example.com BATCH +2 whois-without-numeric-end
Server: @batch=2 :irc.example.com 311 client nick ~ident host * :Name
Server: @batch=2 :irc.example.com 671 client nick :is using a secure connection
Server: @batch=1 :irc.example.com BATCH -2
Server: :irc.example.com BATCH -1
@DarthGandalf

DarthGandalf Jul 13, 2015

Member

If both caps are enabled, server would need to send nested batches, to be able to provide both batch types...

Client: @label=mGhe5V7RTV WHOIS nick
Server: @label=mGhe5V7RTV :irc.example.com BATCH +1 labeled-response
Server: @batch=1 :irc.example.com BATCH +2 whois-without-numeric-end
Server: @batch=2 :irc.example.com 311 client nick ~ident host * :Name
Server: @batch=2 :irc.example.com 671 client nick :is using a secure connection
Server: @batch=1 :irc.example.com BATCH -2
Server: :irc.example.com BATCH -1

This comment has been minimized.

@grawity

grawity Jul 13, 2015

Contributor

Well yes, but without BATCH, how would you know where a @label=asdfg series ends?

Actually, no, I'm not sure how nested batches are necessary here?

@grawity

grawity Jul 13, 2015

Contributor

Well yes, but without BATCH, how would you know where a @label=asdfg series ends?

Actually, no, I'm not sure how nested batches are necessary here?

This comment has been minimized.

@DarthGandalf

DarthGandalf Jul 13, 2015

Member

How to specify 2 batch types (labeled-response, and
whois-without-numeric-end) in a single batch?

2015-07-13 8:42 GMT+01:00 Mantas Mikulėnas notifications@github.com:

In core/labeled-replies-3.3.md
#162 (comment)
:

+If the message doesn't require any response, an empty batch MUST be sent.
+
+## Examples
+
+1. echo-message
+

  • Client: @Label=pQraCjj82e PRIVMSG #channel :\x02Hello!\x02
  • Server: @Label=pQraCjj82e :nick!user@host PRIVMSG #channel :Hello!

+2. WHOIS
+

  • Client: @Label=mGhe5V7RTV WHOIS nick
  • Server: @Label=mGhe5V7RTV :irc.example.com BATCH +NMzYSq45x labeled-response
  • Server: @Batch=NMzYSq45x :irc.example.com 311 client nick ~ident host * :Name
  • ...
  • Server: @Batch=NMzYSq45x :irc.example.com 318 client nick :End of /WHOIS list.

Well yes, but without BATCH, how would you know where a @Label=asdfg
series ends?


Reply to this email directly or view it on GitHub
https://github.com/ircv3/ircv3-specifications/pull/162/files#r34438927.

@DarthGandalf

DarthGandalf Jul 13, 2015

Member

How to specify 2 batch types (labeled-response, and
whois-without-numeric-end) in a single batch?

2015-07-13 8:42 GMT+01:00 Mantas Mikulėnas notifications@github.com:

In core/labeled-replies-3.3.md
#162 (comment)
:

+If the message doesn't require any response, an empty batch MUST be sent.
+
+## Examples
+
+1. echo-message
+

  • Client: @Label=pQraCjj82e PRIVMSG #channel :\x02Hello!\x02
  • Server: @Label=pQraCjj82e :nick!user@host PRIVMSG #channel :Hello!

+2. WHOIS
+

  • Client: @Label=mGhe5V7RTV WHOIS nick
  • Server: @Label=mGhe5V7RTV :irc.example.com BATCH +NMzYSq45x labeled-response
  • Server: @Batch=NMzYSq45x :irc.example.com 311 client nick ~ident host * :Name
  • ...
  • Server: @Batch=NMzYSq45x :irc.example.com 318 client nick :End of /WHOIS list.

Well yes, but without BATCH, how would you know where a @Label=asdfg
series ends?


Reply to this email directly or view it on GitHub
https://github.com/ircv3/ircv3-specifications/pull/162/files#r34438927.

This comment has been minimized.

@DarthGandalf

DarthGandalf Jul 13, 2015

Member

On the other hand...
When response contains single line, it's not required to be in a batch at
all, so the batch type isn't necessary.
So just whois-without-numeric-end should probably be enough, without need
for nested batch.

2015-07-13 8:42 GMT+01:00 Mantas Mikulėnas notifications@github.com:

In core/labeled-replies-3.3.md
#162 (comment)
:

+If the message doesn't require any response, an empty batch MUST be sent.
+
+## Examples
+
+1. echo-message
+

  • Client: @Label=pQraCjj82e PRIVMSG #channel :\x02Hello!\x02
  • Server: @Label=pQraCjj82e :nick!user@host PRIVMSG #channel :Hello!

+2. WHOIS
+

  • Client: @Label=mGhe5V7RTV WHOIS nick
  • Server: @Label=mGhe5V7RTV :irc.example.com BATCH +NMzYSq45x labeled-response
  • Server: @Batch=NMzYSq45x :irc.example.com 311 client nick ~ident host * :Name
  • ...
  • Server: @Batch=NMzYSq45x :irc.example.com 318 client nick :End of /WHOIS list.

Well yes, but without BATCH, how would you know where a @Label=asdfg
series ends?


Reply to this email directly or view it on GitHub
https://github.com/ircv3/ircv3-specifications/pull/162/files#r34438927.

@DarthGandalf

DarthGandalf Jul 13, 2015

Member

On the other hand...
When response contains single line, it's not required to be in a batch at
all, so the batch type isn't necessary.
So just whois-without-numeric-end should probably be enough, without need
for nested batch.

2015-07-13 8:42 GMT+01:00 Mantas Mikulėnas notifications@github.com:

In core/labeled-replies-3.3.md
#162 (comment)
:

+If the message doesn't require any response, an empty batch MUST be sent.
+
+## Examples
+
+1. echo-message
+

  • Client: @Label=pQraCjj82e PRIVMSG #channel :\x02Hello!\x02
  • Server: @Label=pQraCjj82e :nick!user@host PRIVMSG #channel :Hello!

+2. WHOIS
+

  • Client: @Label=mGhe5V7RTV WHOIS nick
  • Server: @Label=mGhe5V7RTV :irc.example.com BATCH +NMzYSq45x labeled-response
  • Server: @Batch=NMzYSq45x :irc.example.com 311 client nick ~ident host * :Name
  • ...
  • Server: @Batch=NMzYSq45x :irc.example.com 318 client nick :End of /WHOIS list.

Well yes, but without BATCH, how would you know where a @Label=asdfg
series ends?


Reply to this email directly or view it on GitHub
https://github.com/ircv3/ircv3-specifications/pull/162/files#r34438927.

This comment has been minimized.

@grawity

grawity Jul 13, 2015

Contributor

Yes, exactly.

labeled-response batches seem completely pointless here (and even everywhere).

@grawity

grawity Jul 13, 2015

Contributor

Yes, exactly.

labeled-response batches seem completely pointless here (and even everywhere).

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Jul 18, 2015

Member

Note to self: mention that client SHOULD NOT reuse the same id until it got response for that id from server.

Member

DarthGandalf commented Jul 18, 2015

Note to self: mention that client SHOULD NOT reuse the same id until it got response for that id from server.

@Mikaela Mikaela referenced this pull request in znc/znc Sep 9, 2015

Open

route_replies as a global/user module #467

core/labeled-replies-3.3.md
+For messages from client which are tagged with `label`, server MUST send exactly
+one message tagged with the same `label` value in response to such message.
+If response contains more than one message, `batch` can be used to group them.
+In that case start of the batch MUST be tagged with the label, and the batch type MUST be `labeled-response`.

This comment has been minimized.

@DanielOaks

DanielOaks Nov 11, 2015

Member

As discussed below, maybe something like:

In that case start of the batch MUST be tagged with the label. The batch type MUST be an existing batch type if it is applicable to the entire response set, orlabeled-responseif one is not.

@DanielOaks

DanielOaks Nov 11, 2015

Member

As discussed below, maybe something like:

In that case start of the batch MUST be tagged with the label. The batch type MUST be an existing batch type if it is applicable to the entire response set, orlabeled-responseif one is not.

@DarthGandalf DarthGandalf referenced this pull request in weechat/weechat Dec 26, 2015

Open

Add support for cap echo-message #139

@BlacklightShining

This comment has been minimized.

Show comment
Hide comment
@BlacklightShining

BlacklightShining Dec 27, 2015

@DarthGandalf What about messages that generate an error and a (potentially modified) echo? I'm thinking of those IRCds that block configured words in messages, but relay the message to Ops in the channel:

Client: @id=4Upxeafq PRIVMSG #channel :A pox on both your houses!
Server: @id=4Upxeafq 000 #channel pox :is a banned word
Server: @id=4Upxeafq :nick!user@example.net PRIVMSG @#channel :A pox on both your houses!

@DarthGandalf What about messages that generate an error and a (potentially modified) echo? I'm thinking of those IRCds that block configured words in messages, but relay the message to Ops in the channel:

Client: @id=4Upxeafq PRIVMSG #channel :A pox on both your houses!
Server: @id=4Upxeafq 000 #channel pox :is a banned word
Server: @id=4Upxeafq :nick!user@example.net PRIVMSG @#channel :A pox on both your houses!
@BlacklightShining

This comment has been minimized.

Show comment
Hide comment
@BlacklightShining

BlacklightShining Dec 27, 2015

Did someone say replace the end of foo numerics? Yes, please.

Did someone say replace the end of foo numerics? Yes, please.

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Apr 7, 2016

Member

What about messages that generate an error and a (potentially modified) echo?

I'd expect them to be in a batch.

Member

DarthGandalf commented Apr 7, 2016

What about messages that generate an error and a (potentially modified) echo?

I'd expect them to be in a batch.

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Apr 7, 2016

Member

Worth keeping in mind how this might interact with server supplied ids #227. That proposal would make more sense as an @id tag, leaving this one as @Label or @request-id or @reqid or something else.

Member

jwheare commented Apr 7, 2016

Worth keeping in mind how this might interact with server supplied ids #227. That proposal would make more sense as an @id tag, leaving this one as @Label or @request-id or @reqid or something else.

kylef and others added some commits Jul 5, 2016

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 24, 2016

Member

Crossposting from #283 (comment)

The labelled-replies spec should probably instruct bouncers to forward the the label tag to all attached clients. We should consider how that interacts with e.g. znc's route_replies.

Member

jwheare commented Nov 24, 2016

Crossposting from #283 (comment)

The labelled-replies spec should probably instruct bouncers to forward the the label tag to all attached clients. We should consider how that interacts with e.g. znc's route_replies.

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 24, 2016

Member

A followup point raised in #283 (comment)

If bouncers are to send labelled replies to all clients, their clients should attempt to use non-overlapping labels and ignore all unrecognised labels.

And the general point from that thread is that clients wishing to not show duplicate (sent+received) messages in PMs to self need to adopt the following behaviour:

  • Ignore all labelled replies if the source nick matches their own nick
  • Clear all temporary local messages displayed

For all other PRIVMSGs, only the label-specified temp message needs to be cleared.


It occurs to me that bouncers that use znc.in/self-message probably don't need to send labelled replies to all clients.

Member

jwheare commented Nov 24, 2016

A followup point raised in #283 (comment)

If bouncers are to send labelled replies to all clients, their clients should attempt to use non-overlapping labels and ignore all unrecognised labels.

And the general point from that thread is that clients wishing to not show duplicate (sent+received) messages in PMs to self need to adopt the following behaviour:

  • Ignore all labelled replies if the source nick matches their own nick
  • Clear all temporary local messages displayed

For all other PRIVMSGs, only the label-specified temp message needs to be cleared.


It occurs to me that bouncers that use znc.in/self-message probably don't need to send labelled replies to all clients.

Rewrite/flesh out labeled-replies.
* adds a wip header
* follows new spec guideline proposal #282
* drops the 3.3 from the text
* changes replies -> responses
* adds an intro/motivation
* starts implementation consideration sections
@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 24, 2016

Member

I rewrote and started fleshing out this spec on a personal branch here:
master...jwheare:labeled

Diff with the current PR (and some commit notes): 0957e98

I left in some outstanding TODOs for implementation considerations and example explanations.

@DarthGandalf let me know if you're happy for me to push that to this PR.

Member

jwheare commented Nov 24, 2016

I rewrote and started fleshing out this spec on a personal branch here:
master...jwheare:labeled

Diff with the current PR (and some commit notes): 0957e98

I left in some outstanding TODOs for implementation considerations and example explanations.

@DarthGandalf let me know if you're happy for me to push that to this PR.

jwheare added a commit to jwheare/ircv3-specifications that referenced this pull request Nov 24, 2016

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Nov 25, 2016

Member

Labels are strictly client-originated. If the message doesn't make sense as a standalone message (but only a response to some request), it should be sent only to the client which did the request.
Bouncer is not required to use the same label for its request to server as what it got from client, it can prefix the label with an internal id of the client.

Labels are strictly client-originated. If the message doesn't make sense as a standalone message (but only a response to some request), it should be sent only to the client which did the request.
Bouncer is not required to use the same label for its request to server as what it got from client, it can prefix the label with an internal id of the client.

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Nov 25, 2016

Member

My motivation was for the bouncer, to know which client to send the responses to.
In that case only bouncer and server need to support this spec, client doesn't need to care.

My motivation was for the bouncer, to know which client to send the responses to.
In that case only bouncer and server need to support this spec, client doesn't need to care.

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 25, 2016

Member

Do you think the client requirements should be separated into a different spec and make this entirely bouncer focused? That wasn't clear to me.

Member

jwheare replied Nov 25, 2016

Do you think the client requirements should be separated into a different spec and make this entirely bouncer focused? That wasn't clear to me.

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Nov 25, 2016

Member

Nah, that's not what I meant.
I needed this spec to replace http://wiki.znc.in/Route_replies with something which actually works.
But the same protocol applies to non-bouncer use case too. I didn't even mention bouncers in my version of it.

Because I didn't have a Motivation section at all...

Member

DarthGandalf replied Nov 25, 2016

Nah, that's not what I meant.
I needed this spec to replace http://wiki.znc.in/Route_replies with something which actually works.
But the same protocol applies to non-bouncer use case too. I didn't even mention bouncers in my version of it.

Because I didn't have a Motivation section at all...

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Nov 25, 2016

Member

Relies how? Does client need to request it from CAP separately? I did specify this.

Relies how? Does client need to request it from CAP separately? I did specify this.

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Nov 25, 2016

Member

Comments above have some ideas what else was missing in earlier version of the spec.
I added a few more comments to your commit.

Member

DarthGandalf commented Nov 25, 2016

Comments above have some ideas what else was missing in earlier version of the spec.
I added a few more comments to your commit.

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 25, 2016

Member

I've pushed some additional changes to my branch to address your commit comments and other points mentioned in this thread.

"Server implementation considerations" still has a TODO in it. Any guidelines we could mention in there?

Member

jwheare commented Nov 25, 2016

I've pushed some additional changes to my branch to address your commit comments and other points mentioned in this thread.

"Server implementation considerations" still has a TODO in it. Any guidelines we could mention in there?

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 25, 2016

Member

Some other points raised by @attilamolnar in IRC:

<Attila> any reason to use gibberish as labels?
<Attila> should limit to numbers in a given range

I don't think we need to restrict the labels beyond being opaque identifiers and the reuse guidelines I added a mention for.

<Attila> can you define what is a "response"
<Attila> e.g. what's the response to JOIN, only the JOIN message that comes back or the entire channel burst sequence
<Attila> what's the response to NICK+USER on reg

In the absence of a clear use case for those, I'm not sure it matters too much. An empty batch for all, or maybe a labelled JOIN response.

<Attila> might make more sense to define for which commands the behavior is required exactly so there's some scope
<Attila> i can think of 2 exact use cases, the first being echo-message and the second is who should znc route the whois to
<Attila> it'd be nice if you could explain why is a completely generic solution needed instead of solving these 2 specfic cases
<Attila> znc has the whois routing problem because whois replies may come out of order or may never arrive, this is not true for other commands such as WHO, MOTD, LUSERS, etc
<Attila> can be true for remote versions of the above commands but still it's only a few commands that allow that

route_replies currently has a list of 16 commands it handles and says "this list is far from complete"
https://github.com/znc/znc/blob/2e6c8d90e71c6ba59e41db1074cd863108a5bc89/modules/route_replies.cpp

Should we be defining the behaviour for all of these as well as the JOIN/USER/NICK examples?

Is there a general rule we can enforce that will cover most cases well enough?

A few other non bouncer specific use cases I can think of:

  • Tracking automated queries like ISON, WHO to prevent displaying the results to users. While MONITOR, WHOX exist, and channelburst may arrive in future, they're not evenly distributed.
  • Tracking which message triggers e.g. unknown nick/channel errors, or other ambiguous errors that can result from multiple different actions.
Member

jwheare commented Nov 25, 2016

Some other points raised by @attilamolnar in IRC:

<Attila> any reason to use gibberish as labels?
<Attila> should limit to numbers in a given range

I don't think we need to restrict the labels beyond being opaque identifiers and the reuse guidelines I added a mention for.

<Attila> can you define what is a "response"
<Attila> e.g. what's the response to JOIN, only the JOIN message that comes back or the entire channel burst sequence
<Attila> what's the response to NICK+USER on reg

In the absence of a clear use case for those, I'm not sure it matters too much. An empty batch for all, or maybe a labelled JOIN response.

<Attila> might make more sense to define for which commands the behavior is required exactly so there's some scope
<Attila> i can think of 2 exact use cases, the first being echo-message and the second is who should znc route the whois to
<Attila> it'd be nice if you could explain why is a completely generic solution needed instead of solving these 2 specfic cases
<Attila> znc has the whois routing problem because whois replies may come out of order or may never arrive, this is not true for other commands such as WHO, MOTD, LUSERS, etc
<Attila> can be true for remote versions of the above commands but still it's only a few commands that allow that

route_replies currently has a list of 16 commands it handles and says "this list is far from complete"
https://github.com/znc/znc/blob/2e6c8d90e71c6ba59e41db1074cd863108a5bc89/modules/route_replies.cpp

Should we be defining the behaviour for all of these as well as the JOIN/USER/NICK examples?

Is there a general rule we can enforce that will cover most cases well enough?

A few other non bouncer specific use cases I can think of:

  • Tracking automated queries like ISON, WHO to prevent displaying the results to users. While MONITOR, WHOX exist, and channelburst may arrive in future, they're not evenly distributed.
  • Tracking which message triggers e.g. unknown nick/channel errors, or other ambiguous errors that can result from multiple different actions.
@attilamolnar

This comment has been minimized.

Show comment
Hide comment
@attilamolnar

attilamolnar Nov 25, 2016

Member

I don't think we need to restrict the labels beyond being opaque identifiers and the reuse guidelines I added a mention for.

I'd like to see them restricted for simplicity. If they are allowed to be opaque strings then corner cases such as length limiting and truncation can happen which can be avoided by restricting them to e.g. a 32-bit unsigned integer which is both reasonable and avoids servers having to add extra sanity checks to ensure it's not too long.

route_replies currently has a list of 16 commands it handles and says "this list is far from complete"
https://github.com/znc/znc/blob/2e6c8d90e71c6ba59e41db1074cd863108a5bc89/modules/route_replies.cpp

Should we be defining the behaviour for all of these as well as the JOIN/USER/NICK examples?

I haven't checked all of those commands but from what I've seen several of those e.g. ISON, WHO, LIST, NAMES, METADATA don't require a labeled-replies like solution to be routed correctly. A reply to ISON is RPL_ISON and the rest have and end numeric. As opposed to e.g. remote WHOIS a bouncer with multiple connected clients could send these commands and expect the replies to come back in the same order as the queries were sent.

Member

attilamolnar commented Nov 25, 2016

I don't think we need to restrict the labels beyond being opaque identifiers and the reuse guidelines I added a mention for.

I'd like to see them restricted for simplicity. If they are allowed to be opaque strings then corner cases such as length limiting and truncation can happen which can be avoided by restricting them to e.g. a 32-bit unsigned integer which is both reasonable and avoids servers having to add extra sanity checks to ensure it's not too long.

route_replies currently has a list of 16 commands it handles and says "this list is far from complete"
https://github.com/znc/znc/blob/2e6c8d90e71c6ba59e41db1074cd863108a5bc89/modules/route_replies.cpp

Should we be defining the behaviour for all of these as well as the JOIN/USER/NICK examples?

I haven't checked all of those commands but from what I've seen several of those e.g. ISON, WHO, LIST, NAMES, METADATA don't require a labeled-replies like solution to be routed correctly. A reply to ISON is RPL_ISON and the rest have and end numeric. As opposed to e.g. remote WHOIS a bouncer with multiple connected clients could send these commands and expect the replies to come back in the same order as the queries were sent.

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Nov 25, 2016

Member

PR updated with my changes.

Member

jwheare commented Nov 25, 2016

PR updated with my changes.

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Dec 2, 2016

Member

I've implemented this along with echo-message in IRCCloud now. The server side was actually quite straightforward.

Each incoming message has a handler that returns a list of 0 or more output lines in an intermediate record format. If the incoming line has a label, the output is either tagged if it's a single line, or wrapped in a batch for more, or an empty batch is sent for 0.

Caps are then checked once before converting to the wire format, and the tags/batches might get optionally filtered out depending on what the client has negotiated. This saves having to spread cap checks throughout the various different parts that add other tags.

There was no need to add any custom command handling, it's all done at a single entry/exit point.

@attilamolnar can you share any details on how you implemented this on the InspIRCD testnet? We should condense our experiences into the server implementation considerations section.

Member

jwheare commented Dec 2, 2016

I've implemented this along with echo-message in IRCCloud now. The server side was actually quite straightforward.

Each incoming message has a handler that returns a list of 0 or more output lines in an intermediate record format. If the incoming line has a label, the output is either tagged if it's a single line, or wrapped in a batch for more, or an empty batch is sent for 0.

Caps are then checked once before converting to the wire format, and the tags/batches might get optionally filtered out depending on what the client has negotiated. This saves having to spread cap checks throughout the various different parts that add other tags.

There was no need to add any custom command handling, it's all done at a single entry/exit point.

@attilamolnar can you share any details on how you implemented this on the InspIRCD testnet? We should condense our experiences into the server implementation considerations section.

jwheare added a commit that referenced this pull request Dec 20, 2016

@jwheare jwheare requested a review from attilamolnar Jan 1, 2017

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Jan 1, 2017

Member

I dropped the empty "Server implementation considerations" section as I can't think what to put in there that doesn't get lost in unnecessary detail/assumptions about various server architectures. Any objections to merging this as-is?

Member

jwheare commented Jan 1, 2017

I dropped the empty "Server implementation considerations" section as I can't think what to put in there that doesn't get lost in unnecessary detail/assumptions about various server architectures. Any objections to merging this as-is?

@grawity

grawity approved these changes Jan 1, 2017

core/labeled-replies-3.3.md
+
+Servers advertising this capability indicate that they are capable of handling the message tag described below from clients, and will use with the same tag and value in their response.
+
+Servers MUST reject the capability at negotiation time if the `batch` capability is not also negotiated. Additionally, servers supporting `cap-notify` MUST cancel the capability if `batch` is removed at any time.

This comment has been minimized.

@attilamolnar

attilamolnar Jan 1, 2017

Member

I don't agree with the limitation imposed by the first sentence because it introduces a special case in cap negotiation in servers that is unnecessary for all clients that behave according to the spec. Additionally, it should be legal for clients to do CAP REQ :foo bar draft/labeled-response baz then CAP REQ :batch without anything bad happening.

@attilamolnar

attilamolnar Jan 1, 2017

Member

I don't agree with the limitation imposed by the first sentence because it introduces a special case in cap negotiation in servers that is unnecessary for all clients that behave according to the spec. Additionally, it should be legal for clients to do CAP REQ :foo bar draft/labeled-response baz then CAP REQ :batch without anything bad happening.

This comment has been minimized.

@jwheare

jwheare Jan 1, 2017

Member

So instead should servers treat draft/labeled-response negotiation as implicitly enabling batch?

@jwheare

jwheare Jan 1, 2017

Member

So instead should servers treat draft/labeled-response negotiation as implicitly enabling batch?

core/labeled-replies-3.3.md
+
+For any message received from a client that includes this tag, the server MUST include the same tag and value in any response required from this message. Servers MUST include the tag in exactly one message.
+
+If a response conists of more than one message, a `batch` MUST be used to group them into a single logical response. The start of the batch MUST be tagged with the `draft/label` tag. The batch type MUST be one of:

This comment has been minimized.

@attilamolnar

attilamolnar Jan 1, 2017

Member

Typo in "conists"

@attilamolnar

attilamolnar Jan 1, 2017

Member

Typo in "conists"

core/labeled-replies-3.3.md
+
+When sending messages directed at a client's own nick, `echo-message` will result in duplicate messages being sent by the server, as both sent and received messages. Labeled responses allow clients to deduplicate these messages in one of two ways:
+
+For private messages that match the clients nickname:

This comment has been minimized.

@attilamolnar

attilamolnar Jan 1, 2017

Member

Typo, should be "client's"

@attilamolnar

attilamolnar Jan 1, 2017

Member

Typo, should be "client's"

This comment has been minimized.

@jwheare

jwheare Jan 1, 2017

Member

It's a plural, not a possessive.

Edit: weird, this was referencing a different line when I first looked.

@jwheare

jwheare Jan 1, 2017

Member

It's a plural, not a possessive.

Edit: weird, this was referencing a different line when I first looked.

core/labeled-replies-3.3.md
+When sending messages directed at a client's own nick, `echo-message` will result in duplicate messages being sent by the server, as both sent and received messages. Labeled responses allow clients to deduplicate these messages in one of two ways:
+
+For private messages that match the clients nickname:
+1. Ignore labeled messages, and use any unlabeled message as acknowledgment for all sent messages to clear temporary local messages.

This comment has been minimized.

@attilamolnar

attilamolnar Jan 1, 2017

Member

When this cap is used together with echo-message and the client messages itself then both messages (one from echo-message and the other one being the delivered message) will be labeled because both of those events were generated because of the labeled PRIVMSG the client sent to the server, i.e. both are "replies". This means "unlabeled messages" don't exist in these cases, or do I misunderstand something?

@attilamolnar

attilamolnar Jan 1, 2017

Member

When this cap is used together with echo-message and the client messages itself then both messages (one from echo-message and the other one being the delivered message) will be labeled because both of those events were generated because of the labeled PRIVMSG the client sent to the server, i.e. both are "replies". This means "unlabeled messages" don't exist in these cases, or do I misunderstand something?

core/labeled-replies-3.3.md
+
+### Tags
+
+This specification adds the `draft/label` message tag, which has one required value.

This comment has been minimized.

@attilamolnar

attilamolnar Jan 1, 2017

Member

Could clarify this, tags either have a value or not have one but the wording here implies there may be multiple values for a tag

@attilamolnar

attilamolnar Jan 1, 2017

Member

Could clarify this, tags either have a value or not have one but the wording here implies there may be multiple values for a tag

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Jan 2, 2017

Member

s/the label tag MUST NOT be included/the server MUST NOT include the label tag/

s/the label tag MUST NOT be included/the server MUST NOT include the label tag/

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Jan 2, 2017

Member

Fixed in f4e22b6

Member

jwheare replied Jan 2, 2017

Fixed in f4e22b6

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Jan 2, 2017

Member

Does it mean this?

-> CAP REQ :draft/labeled-response
<- CAP ACK :batch draft/labeled-response

Does it mean this?

-> CAP REQ :draft/labeled-response
<- CAP ACK :batch draft/labeled-response

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Jan 2, 2017

Member

This whole paragraph got removed in 107607a

Member

jwheare replied Jan 2, 2017

This whole paragraph got removed in 107607a

core/labeled-replies-3.3.md
@@ -40,7 +40,7 @@ Additionally, labeled responses allow bouncers with multiple connected clients t
### Dependencies
-This specification uses the [message tags framework](/specs/core/message-tags-3.2.html) and relies on support for the [`batch`](/specs/extensions/batch-3.2.html) capability which MUST be negotiated at the same time.
+This specification uses the [message tags framework](/specs/core/message-tags-3.2.html) and uses the [`batch`](/specs/extensions/batch-3.2.html) capability which SHOULD be negotiated at the same time.

This comment has been minimized.

@MuffinMedic

MuffinMedic Jan 4, 2017

Contributor

This specification should be independent of the use of batch - if needed, they can be used in tandem and negotiated at the same time. "... and, if needed, the batch capability should be negotiated at the same time"

The draft/label tag can be useful in the absence of batch. While batches are useful and can aid in keeping relevant content together, it should not be required for all uses of the draft/labeled-response message tag. There are instances where it may be necessary to uniquely identify a labeled response for content that does not belong to a batch, such as in ircv3/ircv3-ideas#6.

IRC Discussion

<prawnsalad> ideally, for any command created, it should be i send command X, and i expect a response for command X. error, ack, or response. so the client isnt sat there waiting for something that may or may not actually happen
<MuffinMedic> so, response is the batch, error would be thins like NO_CHATHISTORY_FOUND, ACCESS_DENIED, etc....?
<prawnsalad> "CHATHISTORY ERR_NOTFOUND/ERR_NOACCESS" would be my on-a-whim suggestion
<dx> space after ERR
<prawnsalad> "CHATHISTORY ERR <err name>" dx, yea this would be better
<MuffinMedic> that prevents a new issue though
<MuffinMedic> what if they request multiple chat histories? it would need to include a random ID parameter in the chathistory command that the server returns with the error
<dx> MuffinMedic: the labeled replies spec can take care of that
<MuffinMedic> dx: so send "@label=ID SCROLLBACK target.." then return "@label=ID SCROLLBACK ERR msg"?
<dx> yeah
@MuffinMedic

MuffinMedic Jan 4, 2017

Contributor

This specification should be independent of the use of batch - if needed, they can be used in tandem and negotiated at the same time. "... and, if needed, the batch capability should be negotiated at the same time"

The draft/label tag can be useful in the absence of batch. While batches are useful and can aid in keeping relevant content together, it should not be required for all uses of the draft/labeled-response message tag. There are instances where it may be necessary to uniquely identify a labeled response for content that does not belong to a batch, such as in ircv3/ircv3-ideas#6.

IRC Discussion

<prawnsalad> ideally, for any command created, it should be i send command X, and i expect a response for command X. error, ack, or response. so the client isnt sat there waiting for something that may or may not actually happen
<MuffinMedic> so, response is the batch, error would be thins like NO_CHATHISTORY_FOUND, ACCESS_DENIED, etc....?
<prawnsalad> "CHATHISTORY ERR_NOTFOUND/ERR_NOACCESS" would be my on-a-whim suggestion
<dx> space after ERR
<prawnsalad> "CHATHISTORY ERR <err name>" dx, yea this would be better
<MuffinMedic> that prevents a new issue though
<MuffinMedic> what if they request multiple chat histories? it would need to include a random ID parameter in the chathistory command that the server returns with the error
<dx> MuffinMedic: the labeled replies spec can take care of that
<MuffinMedic> dx: so send "@label=ID SCROLLBACK target.." then return "@label=ID SCROLLBACK ERR msg"?
<dx> yeah

This comment has been minimized.

@jwheare

jwheare Jan 4, 2017

Member

SHOULD means recommended not required.

@jwheare

jwheare Jan 4, 2017

Member

SHOULD means recommended not required.

This comment has been minimized.

@jwheare

jwheare Jan 4, 2017

Member

To clarify further, the spec doesn't say batches must be used every time, only when multiple lines (or none) are in the response and only if batch has been negotiated.

@jwheare

jwheare Jan 4, 2017

Member

To clarify further, the spec doesn't say batches must be used every time, only when multiple lines (or none) are in the response and only if batch has been negotiated.

This comment has been minimized.

@jwheare

jwheare Jan 4, 2017

Member

Also, you're commenting on outdated changes :)

@jwheare

jwheare Jan 4, 2017

Member

Also, you're commenting on outdated changes :)

This comment has been minimized.

@MuffinMedic

MuffinMedic Jan 4, 2017

Contributor

I tried to find the most recent relevant commit that addressed the issue - still finding my way around GitHub in a collaborative sense. I'll always comment on the latest in the future.

SHOULD means recommended not required.

I'm being a bit nit-picky here and the language seems to suggest that it is always recommended, rather than recommended only in cases where it may be needed.

@MuffinMedic

MuffinMedic Jan 4, 2017

Contributor

I tried to find the most recent relevant commit that addressed the issue - still finding my way around GitHub in a collaborative sense. I'll always comment on the latest in the future.

SHOULD means recommended not required.

I'm being a bit nit-picky here and the language seems to suggest that it is always recommended, rather than recommended only in cases where it may be needed.

This comment has been minimized.

@jwheare

jwheare Jan 4, 2017

Member

No probs, I find it better to comment on the "files changed" view rather than a specific commit.

SHOULD means exactly that :) Strongly recommended where it makes sense. It's a stronger recommendation for server than clients really, but clients will have a better time if they enable it too.

@jwheare

jwheare Jan 4, 2017

Member

No probs, I find it better to comment on the "files changed" view rather than a specific commit.

SHOULD means exactly that :) Strongly recommended where it makes sense. It's a stronger recommendation for server than clients really, but clients will have a better time if they enable it too.

@MuffinMedic

This comment has been minimized.

Show comment
Hide comment
@MuffinMedic

MuffinMedic Jan 5, 2017

Contributor

Looking over this spec again - what advantage / use cases does it have over #285 and #288 ? If we allow for the original sender to include a draft/msgid for any event, and allow replies to any event, an additional label tag seems not to add any new functionality - it only serves to add an additional tag that can be used as a unique identifier, which is already covered in the above specifications.

Since there is no specified format for any of the unique identifiers, as long as it's something that's guaranteed to be unique (as best as possible), a single "message identifier" spec would be ideal. Client sends an event to the server with this event ID, server handles it as appropriate, and forwards it to other clients with the same ID. Likewise, a server-generated event would include an ID that is sent to all clients.

See comment on #285

Contributor

MuffinMedic commented Jan 5, 2017

Looking over this spec again - what advantage / use cases does it have over #285 and #288 ? If we allow for the original sender to include a draft/msgid for any event, and allow replies to any event, an additional label tag seems not to add any new functionality - it only serves to add an additional tag that can be used as a unique identifier, which is already covered in the above specifications.

Since there is no specified format for any of the unique identifiers, as long as it's something that's guaranteed to be unique (as best as possible), a single "message identifier" spec would be ideal. Client sends an event to the server with this event ID, server handles it as appropriate, and forwards it to other clients with the same ID. Likewise, a server-generated event would include an ID that is sent to all clients.

See comment on #285

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Jan 5, 2017

Member

This label is only sent back to the original sender, so it doesn't need any sever-wide uniqueness guarantee. These two specs serve very different purposes.

Member

jwheare commented Jan 5, 2017

This label is only sent back to the original sender, so it doesn't need any sever-wide uniqueness guarantee. These two specs serve very different purposes.

@DarthGandalf

This comment has been minimized.

Show comment
Hide comment
@DarthGandalf

DarthGandalf Jan 5, 2017

Member
Member

DarthGandalf commented Jan 5, 2017

@grawity

This comment has been minimized.

Show comment
Hide comment
@grawity

grawity Jan 6, 2017

Contributor

@MuffinMedic I'd say the two specs work at different layers; they just happen to share the same tag space.

That is, label is a low-level feature for just about any command, with temporary client-specific IDs acting transaction markers, while msgid is high-level metadata for the message itself, with global IDs which can be stored or forwarded.

For example, if you sent a labelled request to replay some chat history, the server would be required to both return the current label and the original msgid when replaying an old message. If both were merged into one tag, that would become a bit more difficult. (That might not be a very good example, considering that we have batches and stuff, but hopefully you got the point anyway?)

Contributor

grawity commented Jan 6, 2017

@MuffinMedic I'd say the two specs work at different layers; they just happen to share the same tag space.

That is, label is a low-level feature for just about any command, with temporary client-specific IDs acting transaction markers, while msgid is high-level metadata for the message itself, with global IDs which can be stored or forwarded.

For example, if you sent a labelled request to replay some chat history, the server would be required to both return the current label and the original msgid when replaying an old message. If both were merged into one tag, that would become a bit more difficult. (That might not be a very good example, considering that we have batches and stuff, but hopefully you got the point anyway?)

@attilamolnar

This comment has been minimized.

Show comment
Hide comment
@attilamolnar

attilamolnar Jan 6, 2017

Member

@attilamolnar can you share any details on how you implemented this on the InspIRCD testnet? We should condense our experiences into the server implementation considerations section.

The core logic is this:
Whenever a command that came from a user that has this tag is executed the module remembers which user is the one running the command. Whenever an IRC message is sent during the execution of the command the destination of the message is checked. If the destination is the user executing the command the message is considered to be a response.

There are some extra checks e.g. to ensure something (empty batch) is always sent and remote commands need extra handling.

The new behavior introduced for self PRIVMSGs recently steps on the toes of the above approach because both the echo message PRIVMSG and the other one is sent while the PRIVMSG command from the client is executed so both are considered replies.

Member

attilamolnar commented Jan 6, 2017

@attilamolnar can you share any details on how you implemented this on the InspIRCD testnet? We should condense our experiences into the server implementation considerations section.

The core logic is this:
Whenever a command that came from a user that has this tag is executed the module remembers which user is the one running the command. Whenever an IRC message is sent during the execution of the command the destination of the message is checked. If the destination is the user executing the command the message is considered to be a response.

There are some extra checks e.g. to ensure something (empty batch) is always sent and remote commands need extra handling.

The new behavior introduced for self PRIVMSGs recently steps on the toes of the above approach because both the echo message PRIVMSG and the other one is sent while the PRIVMSG command from the client is executed so both are considered replies.

@attilamolnar

This comment has been minimized.

Show comment
Hide comment
@attilamolnar

attilamolnar Jan 7, 2017

Member

To clarify, I'm ok with merging this with the current requirements for self PRIVMSGs and will tweak my implementation to support that.

Member

attilamolnar commented Jan 7, 2017

To clarify, I'm ok with merging this with the current requirements for self PRIVMSGs and will tweak my implementation to support that.

@jwheare jwheare merged commit 0165967 into ircv3:master Jan 7, 2017

@jwheare jwheare added ids tags labels Jan 7, 2017

@jwheare jwheare added this to the Roadmap milestone Jan 7, 2017

@MuffinMedic MuffinMedic referenced this pull request in ircv3/ircv3-ideas Feb 13, 2017

Open

History searching #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment