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.

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

Ratify labeled-response #378

Closed
wants to merge 1 commit into from
Closed

Conversation

@DanielOaks
Copy link
Member

@DanielOaks DanielOaks commented Apr 9, 2019

There's been a few server implementations of this and it looks pretty well stable from everything I've seen.


updated 2020-01-17 by @jwheare to add new blocker PR

  • #385 Label revisions (merged)
  • #386 ACK command for empty labeled responses
  • #400 Make batch a hard dependency

Known implementations

Unchecked means incomplete or an intent to implement has been expressed. The numbers indicate minimum requirements. Any others?

Server (2/2)

Client (2/2)

Bouncer

Bots

Networks

@jwheare
Copy link
Member

@jwheare jwheare commented Apr 10, 2019

Still need to resolve this ircv3/ircv3-ideas#37

@jwheare jwheare added this to the Roadmap milestone Apr 29, 2019
@jwheare jwheare mentioned this pull request May 29, 2019
@jwheare
Copy link
Member

@jwheare jwheare commented May 29, 2019

Added PRs to address the blockers: #385 #386

@jesopo
Copy link

@jesopo jesopo commented Jun 12, 2019

Both blockers are now merged

@jwheare
Copy link
Member

@jwheare jwheare commented Jun 12, 2019

0.2 needs more implementations, I'll update the checkboxes.

@SadieCat
Copy link
Contributor

@SadieCat SadieCat commented Jun 12, 2019

I have a 0.2 implementation for InspIRCd I just need to fix a few minor issues with it.

Update InspIRCd's implementation is now updated to 0.2, will be on the testnet shortly.

@slingamn
Copy link
Contributor

@slingamn slingamn commented Jun 13, 2019

I implemented this as ergochat/ergo#557, but it missed the freeze deadline for our upcoming release (v1.1.0), so it'll have to go in after that. I can put it on darwin as soon as it hits the master branch.

@RyanSquared
Copy link
Contributor

@RyanSquared RyanSquared commented Jun 13, 2019

With regards to which sections of a batch should include a label, either the start, or the whole message:

07:04 <LordRyan> i think it would probably be better to say "clients shouldn't rely on messages inside the batch being tagged" because i have a feeling some server implementation is going to just tag all the messages, then wrap it in a batch and tag that batch as well
07:05 <LordRyan> so it wouldn't be a good idea to say something like "servers should not include label tags inside the batch", but it's important for clients to be capable of handling the behavior just in case

@jesopo
Copy link

@jesopo jesopo commented Jun 13, 2019

If batch has not been enabled, the tag MAY be included on only one of the messages in the response.

I am quite uncomfortable with this. Can't we just demand that labeled-response comes with batch support?

<LordRyan> everyone who supports labeled-response supports batch (server-side), so I'd be comfortable with that

@jwheare
Copy link
Member

@jwheare jwheare commented Sep 4, 2019

From what I can tell, there's still an unresolved decision around this. Should batch be a hard requirement for this to be enabled, or do we need more clarity around where tags are sent and how they fallback.

In the past, we've tries to resist too many hard requirements on other specs, but is this a case where it would save a lot of uncertainty of implementation? Thoughts from tech board members please.

@kylef
Copy link
Contributor

@kylef kylef commented Sep 22, 2019

I've implemented draft/labeled-response-0.2 in Palaver, I've discovered a couple of problems along the way:

@jwheare
Copy link
Member

@jwheare jwheare commented Sep 23, 2019

@kylef thanks for the report. ACKs should be working on bnc now as expected.

@syzop
Copy link

@syzop syzop commented Dec 23, 2019

Figured I would re-visit this issue after a while. First of all, UnrealIRCd 5.0.0 (released Dec 13, 2019) ships with draft/labeled-response-0.2 enabled by default.

From what I can tell, there's still an unresolved decision around this. Should batch be a hard requirement for this to be enabled, or do we need more clarity around where tags are sent and how they fallback.
In the past, we've tries to resist too many hard requirements on other specs, but is this a case where it would save a lot of uncertainty of implementation? Thoughts from tech board members please.

Indeed, and I am thankful for not making too many hard requirements. I think that's a good thing.

In this particular case, I think batch should be a hard requirement because without BATCH the labeled-response functionality cannot work properly. It pretty much means multi-line response is broken without BATCH. So, for that reason I would think BATCH should be a hard requirement.

What did others say?

@jwheare
Copy link
Member

@jwheare jwheare commented Dec 23, 2019

No other opinions expressed fwict. But I'm happy to go forward with hard requiring BATCH for this.

@slingamn
Copy link
Contributor

@slingamn slingamn commented Dec 23, 2019

I agree.

I think there is no need to police this requirement (e.g., by having the server demand that batch is requested before labeled-response, or on the same CAP REQ line), but message-tags and batch are basic infrastructure that I think we should be comfortable in requiring people to build on top of.

@SadieCat
Copy link
Contributor

@SadieCat SadieCat commented Jan 3, 2020

I've updated the InspIRCd implementation to require batch.

@DarthGandalf
Copy link
Member

@DarthGandalf DarthGandalf commented Jan 3, 2020

What should happen in the following scenario?

  1. Client sends a labeled message to bouncer, e.g. WHOIS foo
  2. Bouncer sends it to server
  3. Server times out

Should bouncer reply to WHOIS with ACK?

@syzop
Copy link

@syzop syzop commented Jan 3, 2020

I think that is a similar case of what we (including you) discussed in #380 where I describe a hypothetical case of a remote server disconnecting half-way causing a BATCH never to end.
The way I would summarize that thread - although it might not have been written in there as such - is that it is a corner case and it's OK for these to sometimes be unhandled.

@DarthGandalf
Copy link
Member

@DarthGandalf DarthGandalf commented Jan 3, 2020

Ah, while rereading the doc today, I've missed the "where it is feasible to do so" part.

@jwheare
Copy link
Member

@jwheare jwheare commented Jan 17, 2020

Once #400 is merged this should be ready to ratify. Any final thoughts from anyone?

@jwheare
Copy link
Member

@jwheare jwheare commented Jan 18, 2020

I merged #400. I'll take a look at #401 tomorrow then when should be good to go.

@jwheare
Copy link
Member

@jwheare jwheare commented Jan 28, 2020

This was reopened in #403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants