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

InitiateAndSend() / RendezvousAndSend() #214

Merged
merged 2 commits into from Sep 5, 2018
Merged

InitiateAndSend() / RendezvousAndSend() #214

merged 2 commits into from Sep 5, 2018

Conversation

britram
Copy link
Contributor

@britram britram commented Sep 3, 2018

This is my attempt to address #112 with something that's (IMO) less clunky in the common case than #124.

I'm not a big fan of this way of doing things either -- specifically, I think the semantics of RendezvousAndSend(), while consistent, are probably wrong.

mwelzl
mwelzl approved these changes Sep 3, 2018
Copy link
Contributor

@csperkins csperkins left a comment

I don't mind InitiaiteAndSend(), although I'd bike-shed the name, but RendezvousAndSend() needs more thought...


~~~
Connection := Preconnection.InitiateAndSend(messageData, messageContext)
~~~
Copy link
Contributor

@csperkins csperkins Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the general approach of InitiateAndSend. It keeps the types right, and avoids issues with trying to call Send() on Preconnection objects and with enqueued data.

Maybe suggest changing the name to InitiateAndSendIdempotent() though, to be explicit? There are security issues with idempotent data, so we need to (try to) make sure people don't use without thinking through the consequences.

Copy link
Contributor

@abrunstrom abrunstrom Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, being explicit here would be important.

Copy link
Contributor Author

@britram britram Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to change to InitiateAndSendIdempotent(), since it forces that message property.

Copy link
Contributor Author

@britram britram Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<bikeshed> How about InitiateWithIdempotentSend()? </bikeshed>

Copy link
Contributor

@csperkins csperkins Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wfm

establishment.

## Send on Rendezvous: RendezvousAndSend {#rendezvous-and-send}

Copy link
Contributor

@csperkins csperkins Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we need to think more about the rendezvous model.

If the goal is to support traditional TCP simultaneous open, then 0-RTT idempotent data could work. For something like an ICE-driven rendezvous, with NAT traversal, I wonder if it still makes sense? STUN doesn't support delivering data in the binding discovery messages, so it's no longer 0-RTT. And, if some future protocol did support data in binding discovery messages, there's no guarantee that all those messages hit the same server, so the semantics get weird.

I wonder if we should not try to support idempotent, 0-RTT, rendezvous for the first version?

Copy link
Contributor

@abrunstrom abrunstrom Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to leave it out for now.

Copy link
Contributor Author

@britram britram Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tried this out primarily to see what would happen if RendezvousAndSend() were kept consistent with InitiateAndSend() and you get this weird buffered-banner behavior that (1) probably doesn't do anything anyone wants and (2) also probably doesn't work anyway.

Will cut it for now, and we can leave rendezvous acceleration as an area for future research.

@britram britram merged commit 79eccf6 into master Sep 5, 2018
2 checks passed
@britram britram deleted the initiate-and-send-112 branch Sep 5, 2018
@mirjak
Copy link
Contributor

@mirjak mirjak commented Sep 6, 2018

After some discussion with Brian I agree that the only solution is to provide some initial date to the preconnection when calling initiate. However, why do we need to have two different function. If we only have initiate and provide idempotent data as an optional input parameter that should be fine, right?

@britram
Copy link
Contributor Author

@britram britram commented Sep 6, 2018

@mirjak: your question is now #224

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

Successfully merging this pull request may close these issues.

None yet

5 participants