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

Hare msg validation #572

Merged
merged 14 commits into from Feb 25, 2019
Merged

Hare msg validation #572

merged 14 commits into from Feb 25, 2019

Conversation

@gavraz
Copy link
Contributor

@gavraz gavraz commented Feb 19, 2019

  • Extracted validation
  • Instance id is now uint32
  • Broker validates instance id
  • Fixed & added unit tests
@gavraz gavraz force-pushed the hare_msg_validation branch from 85e0376 to c4cf030 Feb 19, 2019
hare/haretypes.go Outdated Show resolved Hide resolved
hare/haretypes.go Outdated Show resolved Hide resolved
hare/algorithm.go Outdated Show resolved Hide resolved
Copy link
Member

@antonlerner antonlerner left a comment

Please refrain from using explicit uint32 types, only cast to primitive type when serializing

}
broker.pending[instanceId.Id()] = append(broker.pending[instanceId.Id()], mOut)
broker.pending[instanceId.Id()] = append(broker.pending[instanceId.Id()], hareMsg)

This comment has been minimized.

@antonlerner

antonlerner Feb 19, 2019
Member

are there any concurrent writes to pending[] map? should it be locked if so? I'd recommend running your unit tests with --race before you push

This comment has been minimized.

@gavraz

gavraz Feb 20, 2019
Author Contributor

I am protecting it with the lock. Let me know if you see a scenario in which I do not. Didn't see issues with -race regarding this test.

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

As I wrote in the comment above, if you'll move Register into this select you could lose the mutex. @antonlerner what do you think? It is a different scenario than what you'd benchmarked, Register is called less often than this loop

This comment has been minimized.

@gavraz

gavraz Feb 21, 2019
Author Contributor

I am not sure it is a good idea because I will eventually want to remove the inboxer interface and return the inbox. That will complicate the register call. We can probably solve it by passing a function on the channel but it sounds too complicated.

This comment has been minimized.

@zalmen

zalmen Feb 24, 2019
Contributor

you can still make this change and return the channel. Just create the channel and pass it to the dispatcher via the register channel. You can make this change as well as removing the inboxer as part of this PR.

@gavraz gavraz requested a review from zalmen Feb 20, 2019
hare/broker.go Outdated Show resolved Hide resolved
hare/broker.go Outdated Show resolved Hide resolved
continue
}

instanceId := NewBytes32(hareMsg.Message.InstanceId)
// message validation

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

Consider extracting the code below to a method (until // validation passed), it will improve the readability

This comment has been minimized.

@gavraz

gavraz Feb 21, 2019
Author Contributor

I considered it before and decided that using continue is preferred on using the return value.

This comment has been minimized.

@zalmen

zalmen Feb 24, 2019
Contributor

Your comment has nothing to do with what I wrote... the dispatcher method is very long for no reason. Please break into smaller methods

outbox map[uint32]chan *pb.HareMessage
pending map[uint32][]*pb.HareMessage
mutex sync.RWMutex
maxReg uint32

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

consider renaming to lastReg, "max" is a bit confusing

This comment has been minimized.

@gavraz

gavraz Feb 21, 2019
Author Contributor

But it is the max and not the last. using the last will add the assumption that we never start a proc id' before id if id'>id. While this is correct for now I prefer not to add this assumption, nor see a reason to do that.

This comment has been minimized.

@zalmen

zalmen Feb 24, 2019
Contributor

Of course you'll never start proc id' before id, if id'>id. The assumptions should be as strict as possible and should be loosened only when needed. In which case would you allow executing process out of order? Do you have tests for such cases?

continue
}

if hareMsg.Message.InstanceId > broker.maxReg+1 { // intended for future unregistered instance

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

you can't read from maxReg without locking broker.mutex. I would suggest that you'll move Register and Unregister into this select loop and lose the mutex

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

Also, you should also check that the InstanceId belongs to a "live" execution (i.e. that if someone tries to send you a message on a Hare instance that was already completed that message will be considered invalid)

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

btw, currently it seems that no one calls Unregister - open a bug (if it's a small fix then add it as part of this PR)

This comment has been minimized.

@gavraz

gavraz Feb 21, 2019
Author Contributor

Fixed 1 & 2.
Opened a bug for 3.

}
broker.pending[instanceId.Id()] = append(broker.pending[instanceId.Id()], mOut)
broker.pending[instanceId.Id()] = append(broker.pending[instanceId.Id()], hareMsg)

This comment has been minimized.

@zalmen

zalmen Feb 21, 2019
Contributor

As I wrote in the comment above, if you'll move Register into this select you could lose the mutex. @antonlerner what do you think? It is a different scenario than what you'd benchmarked, Register is called less often than this loop

@gavraz gavraz force-pushed the hare_msg_validation branch from 5de3d44 to 757b188 Feb 21, 2019
@gavraz gavraz requested a review from zalmen Feb 24, 2019
@gavraz gavraz force-pushed the hare_msg_validation branch from 2a3e211 to 9df43b9 Feb 25, 2019
@zalmen
zalmen approved these changes Feb 25, 2019
hare/algorithm.go Show resolved Hide resolved
hare/broker.go Outdated Show resolved Hide resolved
@gavraz gavraz dismissed antonlerner’s stale review Feb 25, 2019

Fixed & reviewed by Yuval

@gavraz gavraz merged commit 664c6bf into develop Feb 25, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
beckmani added a commit that referenced this pull request Mar 6, 2019
* Extracted validation
* Instance id is now uint32 like layer id
* Broker validates instance id
* Replaced uint32 with InstanceId and id types
* Event loop broker (no mutex)
@y0sher y0sher deleted the hare_msg_validation branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants