mashupbots / socko

A Scala web server powered by Netty networking and AKKA processing.
Other
255 stars 51 forks source link

Add information about the need to release netty's buffers after message processing in WebSocket #112

Open yeputons opened 9 years ago

yeputons commented 9 years ago

Say, websocker example does not call frame.frame.release() after it finishes processing, which results in memory leak. You can read more about reference-counted objects in Netty here.

One possible way to check that you have no easy-detectable memory leaks is to run the application with -Dio.netty.leakDetectionLevel=paranoid, run garbage collector a lot with System.gc() and look for errors in logs.

simbo1905 commented 9 years ago

Can you point out the example code which has the problem and provide a gist of the fix?

simbo1905 commented 9 years ago

So what I am reading is that the change would look like this in a typical socko app https://github.com/simbo1905/sprint-planning/commit/6faec3fe4173e0dafeee2590fb447d81bca8f727

Do we have to leave it down to the person writing the app to remember to call release? Would it not be better and safer for socko to always call release after it has run the application supplied code?

yeputons commented 9 years ago

@simbo1905 Yep, that's what it looks like. However, some applications may want to pass buffers internally: say, to other actors. So if Socko suddenly start releasing buffer, it will be very surprising backward-incompatible change - apps will either crash or get wrong data because the buffer they try to access is already recycled.

simbo1905 commented 9 years ago

@yeputons okay but the vanilla use case is to parse the buffer into some app immutable type then pass on the app type for processing freeing the buffer. handing the release for the vanilla use case is error prone as exceptions may be thrown. how about this version which has a releaseAfter(f) { doNormalWork }? if that looks usable then i would suggest you or i should fork the demo code here and create a PR to have that helper function merged back into this repo.