Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

Rework transport tag allocation logic #15

Merged
merged 4 commits into from
May 25, 2016

Conversation

kennylevinsen
Copy link
Contributor

New tag allocation logic that avoids tag collisions, and errors out if a tag cannot be allocated.

@stevvooe
Copy link
Contributor

cc @justincormack

// about outstanding requests.
tags++
fcall := newFcall(tags, req.message)
var selected Tag
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind compartmentalizing this into a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@kennylevinsen
Copy link
Contributor Author

Summary of changes:

  1. Tag allocation logic extracted to a method.
  2. Tag hint to avoid unnecessary iteration (re)introduced.
  3. newFcall tag reset logic removed. (It's not convenient, and blurs the lines of tag responsibility).

@@ -107,11 +107,6 @@ type Fcall struct {
}

func newFcall(tag Tag, msg Message) *Fcall {
switch msg.Type() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Double check that this doesn't break negotiation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clientnegotiate and servernegotiate both hardcode NOTAG to the call. Negotiation to my rather pedantic server also works with 9pr.

@stevvooe
Copy link
Contributor

@Joushou One check on the tag for negiotiation process and we're good!

@kennylevinsen
Copy link
Contributor Author

I have removed that check, and added commentary that the map must not contain NOTAG (it will otherwise give an erroneous tag pool depletion error when there is still an available tag).

@stevvooe
Copy link
Contributor

LGTM

@justincormack PTAL

@justincormack
Copy link
Contributor

LGTM

@stevvooe
Copy link
Contributor

@Joushou Thanks for the great contribution!

Before merging, we'll need you to sign your commits.

(Sorry, normally, this is automated, but we have yet to set this up for go-p9p)

@kennylevinsen
Copy link
Contributor Author

All 4 commits signed.

New tag allocation system that avoids tag collisions and incorrect
use of NOTAG.

Signed-off-by: Kenny Levinsen <w@kl.wtf>
Defaulting the tag in newFcall might result in unexpected behaviour.
For example, if a client (erroneously) sends a Tversion on a tag, the
server response will not be on a tag. newFcall should just create the
struct.

Signed-off-by: Kenny Levinsen <w@kl.wtf>
Signed-off-by: Kenny Levinsen <w@kl.wtf>
Signed-off-by: Kenny Levinsen <w@kl.wtf>
@stevvooe
Copy link
Contributor

@Joushou Cheers!

@stevvooe stevvooe merged commit 11b5e5c into docker-archive:master May 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants