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

Session expiration not threadsafe #203

Closed
urso opened this issue Aug 20, 2015 · 6 comments
Closed

Session expiration not threadsafe #203

urso opened this issue Aug 20, 2015 · 6 comments
Assignees

Comments

@urso
Copy link

urso commented Aug 20, 2015

Session expiration is not threadsafe.

Documentation for time.AfterFunc says:

waits for the duration to elapse and then calls f in its own goroutine. It returns a Timer that can be used to cancel the call using its Stop method.

The expire functions simply drops the transaction from internal map. But maps are not thread safe. With golang 1.5 release on august 19th with GOMAXPROCS=(number of logical CPUs) by default tables might get corrupted by chance.

Some possible solutions:

  1. use mutex
  2. 'send' expired transaction to main loop for deletion.
  3. create our own table of constant size (delete just overwrites pointer then - atomic op?)
@urso urso added the bug label Aug 20, 2015
@tsg
Copy link
Contributor

tsg commented Aug 20, 2015

I think an effect of this was just reported as #204.

@urso
Copy link
Author

urso commented Aug 20, 2015

yes, traces of goroutine 10 and 1227 fully match the problem. This ticket is just more general, as similar code is different places.

@tsg
Copy link
Contributor

tsg commented Aug 21, 2015

I'm for a getting a fix quicker by putting a mutex around those maps (so solution 1). I'm thinking to make a "map + mutex" type in libbeat.common and provide some utilities so that it's easy to support our use cases. If you agree, I can start working on that.

@urso
Copy link
Author

urso commented Aug 21, 2015

Agreed. Let's identify and fix our multithreading issues first.

@rshriram
Copy link
Contributor

May I suggest to split the map per core to avoid the scalability bottleneck? Because, if one uses AF_PACKET in FANOUT_HASH mode, packets from a flow are delivered to the same core. So, session expiration could be sent out as transaction expired events to the main loop per core.

However, looking at the current code, there is no support for using AF_PACKET in fanout mode (which provides a lot more scalability than the stated 200K packets per second). So some machinery would have to be added to have multiple TPacket sockets and distributing them one per core. Gopacket has AF_PACKET fanout support builtin.

@urso
Copy link
Author

urso commented Aug 21, 2015

Yes, fanout is not supported yet.

For all the different protocols we support the transaction expiration pattern using timeouts is very common. We might want to add some special special expiration (timer) support that helps contributors with these kinds of issues (like multiplexing timers on same goroutine processing packets).

When we add fanout I'm all in for reducing shared state aggressively.

@andrewkroh andrewkroh self-assigned this Sep 9, 2015
tsg added a commit that referenced this issue Sep 18, 2015
Issue #203 - Protecting protocol maps against concurrent modifications.
monicasarbu added a commit that referenced this issue Dec 2, 2015
monicasarbu added a commit that referenced this issue Dec 2, 2015
tsg added a commit to tsg/beats that referenced this issue Jan 20, 2016
…mutex

Issue elastic#203 - Protecting protocol maps against concurrent modifications.
tsg pushed a commit to tsg/beats that referenced this issue Jan 20, 2016
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