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

@draft/label tags and commands that never reply #391

Closed
csmith opened this issue Feb 15, 2019 · 12 comments
Closed

@draft/label tags and commands that never reply #391

csmith opened this issue Feb 15, 2019 · 12 comments
Assignees

Comments

@csmith
Copy link
Contributor

csmith commented Feb 15, 2019

From #oragono:

What's the expected behaviour when you send a @draft/label tag on a message that doesn't generate a reply (like CAP END or PONG)? The spec implies the server will send an empty batch, but Ora just ignores it.

It's a bit nonsensical to label those, but it's a lot easier to just label all outgoing messages than figure out which are sensible things and which aren't...

@slingamn
Copy link
Member

Trying to summarize some discussion:

  1. Question: does the current oragono behavior comply with the specifications?
  2. csmith and jwheare discussed a change to https://ircv3.net/specs/extensions/labeled-response.html that adds ACK/NOOP replies; this should become a ticket on ircv3/ircv3-specifications or ircv3/ircv3-ideas
  3. Clients interacting with legacy servers may be able to simulate this "acknowledgement required" behavior, by sending a PING with a unique string and waiting for PONG (thus providing an acknowledgement from the server that all previous commands were processed, even in cases where a command doesn't send its own reply)

@DanielOaks
Copy link
Member

  1. Current behaviour complies with the specification.
  2. Yepyep, for sure. Any changes to the specification should be made on that end, not by us directly.
  3. Definitely, that makes sense as a thing to recommend as a part of the specification change as well.

@slingamn
Copy link
Member

Assigning to @csmith to look into the spec change.

@csmith
Copy link
Contributor Author

csmith commented Feb 20, 2019

Current behaviour complies with the specification.

Hmm. Maybe I'm misunderstanding the spec. From these two bits:

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 logical message.
...
If no response is required, an empty batch MUST be sent.

It sounds like the required response to, say:

@draft/label=abc123 PONG foo

is:

@draft/label=abc123 BATCH +something labeled-response
BATCH -something

But Ora doesn't return anything to the client.

I'll raise a ticket about introducing an ACK/NOOP in place of BATCH+/BATCH-, once I'm sure I'm not barking up the wrong tree :)

@DanielOaks
Copy link
Member

Strange, we should be sending an empty batch in response to any incoming command so long as the client has the batch cap requested. I'll take a look, thanks.

@slingamn
Copy link
Member

ResponseBuffer has always been very deliberate about not sending batch start and end when the buffer is empty. But yeah --- it certainly looks like this behavior is unambiguously in violation of the spec.

It sounds like the correct and optimal behavior is:

  1. 0 messages in buffer: send batch start with label, then send batch end
  2. 1 message in buffer: send it with label, do not send batch start or end
  3. 2 or more messages in buffer: send batch start with label, send messages with batch tag, send batch end

I'm going to propose this as a release blocker because it shouldn't be too invasive to fix and we want oragono to be usable as a reference implementation for draft specs.

@slingamn slingamn assigned slingamn and unassigned csmith Feb 21, 2019
@slingamn slingamn added the release blocker Blocks release label Feb 21, 2019
@DanielOaks
Copy link
Member

... my mistake. Yeah for sure

@DanielOaks
Copy link
Member

Note to self: Make sure we gate the above behaviour on having a label for the responsebuffer. If they don't supply any labels we can just ignore the 0 messages in buffer case.

@slingamn
Copy link
Member

@csmith I think we should still move forward with an ACK/NOOP spec extension; the 0-message case shouldn't be required to be more verbose than the 1-message case.

With this extension, the correct and optimal behavior would be:

  1. 0 messages in buffer: send labeled ACK/NOOP message, no batch start or end
  2. 1 message in buffer: send it with label, no batch start or end
  3. 2 or more messages in buffer: send batch start with label, send messages with batch tag, send batch end

but sending a batch with 0, 1, or 2+ messages would still be correct.

slingamn added a commit to slingamn/irctest that referenced this issue Feb 21, 2019
slingamn added a commit to slingamn/ergo that referenced this issue Feb 21, 2019
@csmith
Copy link
Contributor Author

csmith commented Feb 21, 2019

I've put up ircv3/ircv3-ideas#37 for the proposed spec change.

If there's general consensus on the idea (or some variation of it) I'll look at implementing it in Ora post-1.0

DanielOaks added a commit that referenced this issue Feb 21, 2019
@DanielOaks
Copy link
Member

Untagging 'release blocker' as #411 has been merged, making our current implementation consistent with the draft spec (we now return the empty batch if there's 0 response lines).

@DanielOaks DanielOaks removed the release blocker Blocks release label Feb 21, 2019
@slingamn
Copy link
Member

I think this is fully resolved by the filing of the ircv3-ideas ticket --- further discussion there.

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

No branches or pull requests

3 participants