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

Breaking Change - All functions that took a Peer now take a *Session #108

Open
wants to merge 26 commits into
base: v2
Choose a base branch
from

Conversation

beatgammit
Copy link
Collaborator

  • allows us to generate session-specific ids

I basically rewrote dealer.go on a rereading of the spec. I was getting errors with AutobahnPython saying that the INVOCATION ID was reused (was causing somewhat random crashes). We were using NewID() to generate this, which generates the ID randomly, so I switched it to a session-specific, incrementing ID as per the spec. I also simplified some of the internal data structures.

Moving the *Session into the Dealer is a breaking change and was needed to generate session-specific identifiers for the fix. We talked about this previously, and we had decided not to do it because there wasn't a reason to complicate matters, but I think this is now necessary to be spec compliant.

If this looks good to you, I can check if our ids are correct elsewhere.

- allows us to generate session-specific ids
@beatgammit
Copy link
Collaborator Author

This is the fix for #85

- fixes a problem when running with AutobahnPython and twisted
- prevent data race; maps aren't safe for concurrent use
@mourad
Copy link
Collaborator

mourad commented May 4, 2016

IMO, it does makes sense to make this API change. I also found this change is needed in order to add smarter external router/broker (i.e. make routing decision based on session data)

The beatgammit:v2 branch has moved forward since this PR was created and now includes a lot of other changes. Could you change this PR to only include the original changes?

beatgammit and others added 14 commits August 1, 2016 13:36
- fixes problems with multiple subscribers to the same topic
- no guarantee they still work, but this makes it easier to run tests
- single rw lock to make eliminate races, could definitely use some
  performance tuning/refactoring
- it seems like this was blocking when the socket was already closed,
  which caused some stability issues over time as goroutines built up
- I noticed a that defaultBroker.Subscribe could block, and the only way
  that could happen is if the Send function in either Unsubscribe
  (unlikely) or Publish (very likely) blocked
- I think there's still a larger problem with transports blocking on the
  Send, but this should at least allow messages to go through if that
  happens
- fixes memory leak
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.

4 participants