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

[reporter] [file] Incoherent thread synchronisation #123

Closed
benbenw opened this Issue Jul 5, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@benbenw

benbenw commented Jul 5, 2016

I don't understand the threading management in io.gravitee.reporter.file.FileReporter.

  • Either the write method could be called from different threads AND the date formatter should not be static.
  • Or it's single threaded (through the disruptor EventHandler) and the buffer re-use may not need a Thread Local cache.

Am I missing something here ?

Extra credit question :
Is the synchronized in the write method here to deal with the stop/ start done from another thread ?
Or to eventually manages concurrent calls to the write method ?

@brasseld

This comment has been minimized.

Member

brasseld commented Jul 5, 2016

Hi @benbenw,

Your comment is really interesting !

FYI, we do not use the LMAX disruptor in Gravitee's versions previous to 0.13 and reporter may be called by multiple threads. This is not the case anymore with LMAX and we have a single thread per reporter.

So, the write method could not be called from different threads meaning that,

  • synchronized keyword should be remove
  • we can keep date formatter declaration as-it
  • thread-local can be removed for string buffer

Is this ok for you ?

@benbenw

This comment has been minimized.

benbenw commented Jul 5, 2016

ok for

  • we can keep date formatter declaration as-it
  • thread-local can be removed for string buffer

For the synchronized removal, you have a better understanding of the gravitee internals ;-)
Can the file reporter component be stopped / started / restarted independently of the gravitee application lifecycle ?
If so you need to ensure that the "reporting" thread (lmax eventhandler) will see the updated writer set by another thread.
Synchronized is a way to do it; AtomicReference, volatile are others.

@brasseld

This comment has been minimized.

Member

brasseld commented Jul 5, 2016

Currently, there is no way to stop a reporter independently of the gravitee gateway lifecycle.
But this is probably something we can think in the future !
I have to admit that I have no idea about a typical case but there is certainly one (and you have probably an idea about it ?)

One more question: you're using Gravitee ? why do you take a look to these internal classes of Gravitee ?

@benbenw

This comment has been minimized.

benbenw commented Jul 5, 2016

Just a pet project of mine...

The file reporter has a memory allocation profile that should be improved.
I could provide a "first step" patch but the long -> Instant -> Date conversion will eventually have to be removed.

btw gravitee is a really interesting solution.
So far for the sources I had a look into, the code is clear and easy to follow.
Keep up the good work !

@brasseld

This comment has been minimized.

Member

brasseld commented Jul 5, 2016

It's good to see this kind of feedback and comments ! Thanks a lot !

I will be more than happy to help you on this patch. I did not take time yet to profile memory allocation at the reporter level. Fell free to update code as necessary !

If you have anything to share about the solution, features, enhancements or just more general feedback, feel free to contact and ask me at david.brassely (at) graviteesource.com

@brasseld

This comment has been minimized.

Member

brasseld commented Jul 7, 2016

@brasseld brasseld closed this Jul 7, 2016

@brasseld brasseld changed the title from [file reporter] Incoherent thread synchronisation in file reporter to [reporter] [file] Incoherent thread synchronisation in file reporter Jul 7, 2016

@brasseld brasseld changed the title from [reporter] [file] Incoherent thread synchronisation in file reporter to [reporter] [file] Incoherent thread synchronisation Jul 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment