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

Send() -> Sent<> mapping underspecified #336

Closed
philsbln opened this issue Jul 4, 2019 · 12 comments
Closed

Send() -> Sent<> mapping underspecified #336

philsbln opened this issue Jul 4, 2019 · 12 comments

Comments

@philsbln
Copy link
Contributor

philsbln commented Jul 4, 2019

PR #321 added a return value to the Send to make matching of send error events and reply messages to their originating message easier:

messageContext := Connection.Send(messageData, messageContext?, endOfMessage?)

@tfpauly raised the concern that this this adds return value that most often is not needed and ignoring it may often trigger a warning.
Therefore, we have to decide whether to keep the return value based on what is worse:

  • Requiring the application to explicitly create a message context if it wants to identify the message that caused a send errors event or was the original request of a reply.
  • Having a return value that is often ignored and causes a lot of warnings in several implementations.
@tfpauly
Copy link
Contributor

tfpauly commented Jul 8, 2019

As stated in the PR, I'd prefer to see Send() not return any value. If you want to be able to look up information about a context, send an explicit context.

The point about receiving send errors is interesting—and I'd argue that the messageContext is not what you want to associate errors with. When you're sending partial messages, you have one MessageContext across multiple send operations. The first 10 sends on a message context can succeed, and then the eleventh can fail. The error should be associated with the call to Send(), not the context. I'm not sure how our pseudocode handles this, but it would effectively be a completion or promise in specific languages.

@philsbln
Copy link
Contributor Author

philsbln commented Jul 9, 2019

The mapping between sends and error events is indeed complicated and worth an issue on its own:

  • Multiple partial sends can easily end up in the same packet – resulting in a single Fail event for a dozen partial sends
  • A singe message context can represent many partial sends
  • A singe message context can represent many packets
  • With message coalescation, even a single packet can be associated with a dozen message contexts (think: QUIC with stream as a message, small requests)

I still think returning a message context on error events is the right thing to do, but mapping it to the message context passed to send will, indeed, be much more complicated.

@tfpauly
Copy link
Contributor

tfpauly commented Jul 9, 2019

It multiple Send calls happen to be sent in a single packet (say a UDP packet), and this hits an error (which for most protocols like TCP doesn't make sense, but you could have an ICMP error for UDP), each Send call should have its own error. The fact that the transport laid out the bytes in a certain way is an implementation detail. There should be a dozen fail events for a dozen failed sends.

I don't think in general a "packet" really has anything to do with the API surface for sending here, and that's a distraction to the API contract.

@philsbln
Copy link
Contributor Author

philsbln commented Jul 10, 2019

I agree that the notion of "packet" does not belong into the API, but thinking about packetization is necessary to understand all error cases.

I am not really convinced that we can and should really match all sends with errors: Let's consider the following (extreme) case:
An application uses partial sends to send a fairly large amount of data byte wise and that data gets transported over TCP. Nagel will coalesce the data into larger packets. Now consider we have a few hundred kilobytes in the send buffer, when a TCP RST for the connection arrives.
What should happen?

  • Send a single connection error
  • Send a single send error indicating the state of the stream
  • Send a few hundred thousand send errors for all bytes in the send buffer
  • Send a few hundred thousand send errors for all bytes that are not acknowledged

@tfpauly
Copy link
Contributor

tfpauly commented Jul 10, 2019

Please see section 7.3.1:

The Sent Event occurs when a previous Send Action has completed, i.e., when the data derived from the Message has been passed down or through the underlying Protocol Stack and is no longer the responsibility of this interface. The exact disposition of the Message (i.e., whether it has actually been transmitted, moved into a buffer on the network interface, moved into a kernel buffer, and so on) when the Sent Event occurs is implementation-specific.

That means that any Send operation that has successfully enqueued its data into the send buffer will have received a Sent event. Acknowledgement is not an event delivered to the application.

In the case you gave, the following will happen upon a RST, as always should happen upon a RST:

  • The connection will report a ConnectionError event with something like ECONNRESET, in addition to:
  • Every outstanding Send operation that was not yet able to enqueue its data in the TCP send buffer due to backpressure will generate a unique SendError event, with something like ECONNRESET or ENOTCONN.

All other Send operations that did enqueue data would have already delivered the Sent event, and are no longer relevant for delivering errors.

@britram britram changed the title Should "send" return a MessageContex? Should "send" return a MessageContext? Jul 22, 2019
@britram
Copy link
Contributor

britram commented Jul 22, 2019

Decided in Montréal: no, send() should not return a MessageContext, it should be passed one.

@mwelzl
Copy link
Contributor

mwelzl commented Jul 22, 2019

I think "it must always be passed one" was the agreed outcome, right?

@tfpauly
Copy link
Contributor

tfpauly commented Jul 22, 2019

Yes, I'm in favor of each Send must have either exactly one Sent Event or a SendError event. We didn't confirm consensus though.

@mwelzl
Copy link
Contributor

mwelzl commented Jul 22, 2019

Rollback. I meant something else, and just created unnecessary confusion, I'm afraid.

  1. I think you said that send() "must always be passed a MessageContext", but it's in fact an optional parameter and should probably stay one.

  2. about "each Send must have exactly one Sent Event or a SendError event", this is a different question... I think I agree with Colin here, who suggested that this should be left up to implementations to decide. (and he said that the current text would even allow for that)

philsbln added a commit that referenced this issue Jul 22, 2019
This closes first half of Issue #336
@britram britram changed the title Should "send" return a MessageContext? Send() -> Sent<> mapping underspecified Jul 24, 2019
@britram
Copy link
Contributor

britram commented Jul 24, 2019

Resolved in Montréal: there is a 1:1 relationship between Send() and Sent<>. Text needs to be clear on this.

@tfpauly
Copy link
Contributor

tfpauly commented Aug 20, 2019

Fixed with 46ec43c

@tfpauly tfpauly closed this as completed Aug 20, 2019
@tfpauly
Copy link
Contributor

tfpauly commented Aug 20, 2019

(Accidentally pushed this to master instead of to a PR branch—pretty small diff. If you want to change something there, let me know!)

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

No branches or pull requests

4 participants