[java] снова memory leaks

Как вообще эти сессии хранятся? Если я сохраняю в сессии что-то (session.setAttribute()), но потом не делаю removeAttribute(), то сессия закроется? Почему она может оставаться даже спустя 20 мин idle-time? Почему GC не очищает эти ConcurrentHashMap'ы? Где там может остаться ссылка?

Метод обновления hashmap - synchronized. Надо ли оборачивать в synchronized блоки везде, где put()/get() используются?

Надо, т.к. пока один поток обновляет коллекцию в синхронизованном методе, другой поток может сделать put()/get() без использования синхронизации. Помимо race condition, это ещё и visibility issue.

Используется только в сервлете. Т.е. спокойно можно делать не static? А треды?

У меня складывается ощущение, что ты считаешь, что static имеет какое-то отношение к многопоточности. Это не так.

Почему GC не очищает эти ConcurrentHashMap'ы?

Почему ты думаешь, что он не очищает? Ты же сам писал, что после вызова GC потребление памяти падает. А GC не обязан каждые 5 минут запускаться. Он вообще не обязан запускаться, пока VM хватает памяти.

Проблема, кажется, все-таки в том, что tomcat хранит старые сессии. Причем, он такой добрый, что сохраняет их даже после restart'а и shutdown'а.

Надо, т.к. пока один поток обновляет коллекцию в синхронизованном методе, другой поток может сделать put()/get() без использования синхронизации. Помимо race condition, это ещё и visibility issue.

Теперь я снова запутался.
Я использую ту мапу как статическую чтобы она 'принадлежала' классу myclassSerlvet, а не отдельным инстансам. Т.е. существовала в 1 экземпляре.

Но, теперь я опять запутался. Допустим, где-то в другом месте у меня тоже используется HashMap (или ArrayList). Как узнать, где надо оборачивать в synchornized блок, а где - нет? Сервлет по дефолту всегда будет создавать несколько тредов?

Я использую ту мапу как статическую чтобы она 'принадлежала' классу myclassSerlvet, а не отдельным инстансам. Т.е. существовала в 1 экземпляре.

У тебя сервлет SingleThreadModel ? Если нет - то должен быть один экземпляр сервлета, который обрабатывает все запросы, каждый запрос в отдельном потоке. Здесь могу ошибаться, но поиск в гугле не опроверг это утверждение.

Допустим, где-то в другом месте у меня тоже используется HashMap (или ArrayList). Как узнать, где надо оборачивать в synchornized блок, а где - нет?

Если с коллекцией работают несколько потоков, и при этом она не read-only - то доступ к ней всегда должен быть синхронизован, либо это должна быть потокобезопасная коллекция.

Сервлет по дефолту всегда будет создавать несколько тредов?

Контейнер для обычного сервлета (не SingleThreadModel) для каждого запроса создаёт свой тред. Возможно, число тредов ограничено сверху. Что происходит при превышении порога не знаю.

Так вот, а как точно узнать сколько с коллекцией будет работать потоков - 1 или больше?

И еще - вот сейчас только метод обновления мапы synchronized. Всякие там put/get'ы - без синхронизации. И вроде как все работает.

Сейчас графики намного лучше (сессии убиваются). Но все равно хочется навести порядок в сервлете с синхронизацией.

Например:

if (myclassMap.get(key) == null ) {
  // создаем новый объект
  myclassMap.put(myclassObject);
}

Пример 2:

MyClass myclassObject = myclassMap.get(key);
...
myclassObject.incSomething();

После того как уменьшил время хранения сессии до 20 мин, перезапустил томкат, картина, вроде, стала намного лучше: http://gyazo.com/e6cb066973e2af15d0c4e5869c477cb3

как точно узнать сколько с коллекцией будет работать потоков - 1 или больше?

От приложения зависит. В сервлетах надо всегде учитывать многопоточность.

Всякие там put/get'ы - без синхронизации. И вроде как все работает.

До поры до времени. Эти ошибки - самые труднообнаруживаемые и устранимые. Видимо, нагрузка не такая большая, либо эти операции достаточно редкие.

Пример 1

А зачем проверять на null? Если объект есть, то новый не будет добавляться? Вообще, такая конструкция называется check-then-act, и не является атомарной, поэтому весь блок, включая if, надо оборачивать. В ConcurrentHashMap есть полезная функция - putIfAbsent, при использовании которой можно обойтись без синхронизации.

Пример 2

Нужно, вызов get. Это нужно для visibility, т.к. если соседний поток сделает put, другой поток может не увидеть обновления, и get вернёт старый объект, либо null (stale data).

Спасибо большое за объяснения! Завтра буду пробовать/править, если что - еще отпишусь :)

Вот тут разбираюсь еще с синхронизацией. Такой вопрос - как это скажется на производительности? Если у меня есть метод, в котором блок if обернут в synchronized, то это значит, что этот блок всегда будет выполняться только в один поток? Или это значит, что лишь при вызове этого метода с Одинаковыми аргументами - сначала выполнится один, потом другой (т.е. оба дойдут до synchronized блока, а там - последовательно)?

В synchronized одновременно может находится только один поток. При большом количестве запросов производительность естественно просядет. Сильно может помочь использовать нормальные коллекции.

И вопрос номер 2:
учитывая то, что класс,в котором сейчас используются статики - является сервлетом, нигде в коде напрямую не создается (ни в одном другом пакете), а создается(как я понимаю) только при первом запросе по определенному адресу (и сервлет должен обработать этот запрос) - вызывается метод init, то статики вообще не нужны? Разные потоки к этому не имеют никакого отношения?

Нормальные коллекции? ConcurrentHashMap вместо HashMap?

И вопрос номер 2:
учитывая то, что класс,в котором сейчас используются статики - является сервлетом, нигде в коде напрямую не создается (ни в одном другом пакете), а создается(как я понимаю) только при первом запросе по определенному адресу (и сервлет должен обработать этот запрос) - вызывается метод init, то статики вообще не нужны? Разные потоки к этому не имеют никакого отношения?

Верно. Инициализируешь данные в методе init. Чтобы первый запрос не тормозил, можешь поставить сервлету, чтобы он создавался при старте контейнера (http://www.javabeat.net/tips/166-load-on-startup-element-in-webxml.html).

Нормальные коллекции? ConcurrentHashMap вместо HashMap?

Да.

В общем, лучше будет если я:
1. Уберу статики (ибо экземпляр сервлета все равно один).
2. Заменю HashMap'ы на ConcurrentHashMap'ы.
3. Уберу все synchronized (ибо теперь будет ConcurrentHashMap'ы)
так?

Да. Только надо учитывать, что ConcurrentHashMap сам по себе не избавит от проблем check-than-act, т.е. надо будет

if (myclassMap.get(key) == null ) {
  // создаем новый объект
  myclassMap.put(myclassObject);
}

Заменять на

// создаем новый объект
myclassObject = new...
concurrentHashMap.putIfAbsent(key, myclassObject);

Ну и в сложных случаях всё же может потребоваться синхронизация. Но операции put()/get() можно будет делать спокойно без синхронизации.

А что за сложные случаи?
И, я так понимаю, метод обновления(который сейчас synchronized) этой мапы можно уже не делать synchronized?

А что за сложные случаи?

Взять что-то из мэпы, изменить, положить обратно. При этом некоторые потоки могут для одного и того же объекта в мэпе попытаться сделать это одновременно. Но это будет проблема со стороны бизнес логики, коллекцию это, конечно, не сломает.

И, я так понимаю, метод обновления(который сейчас synchronized) этой мапы можно уже не делать synchronized?

Смотря как он обновляет. Если только добавляет элементы - можно делать putAll(), если удаляет - removeAll(), если и то и другое, при этом другие потоки не должны видеть промежуточное состояние коллекции - то тут уже нужна синхронизация. Если могут видеть - то не нужна. Но, опять же, коллекцию всё это не сломает.

1. Вообще как сделано: у класса сервлета поле с мапой объявлено как Map<String, MyClass> myclassMap (т.е. interface вместо implementation). В методе же обновления создается implementation мапа (HashMap / ConcurrentHashMap etc.), заполняется, а потом просто myclassMap = та-самая-созданная-и-заполненная-мапа.

2. В сервлете есть таймер. По таймеру выполняется дамп собранных данных в БД. Как тут с синхронизацией? Нужны какие-то пляски? Или таймер всегда будет одним потоком выполняться?

хм...А вот сервлету приходит много одинаковых запросов (примерно в одно время). Он забирает один и тот же объект из мапы, совершает с ним манипуляции (например, увеличивает счетчик), кладет обратно в мапу. Тут тоже беда будет с синхронизацией?

Он забирает один и тот же объект из мапы, совершает с ним манипуляции (например, увеличивает счетчик), кладет обратно в мапу. Тут тоже беда будет с синхронизацией?

Конечно. Два потока взяли объект со счётчиком 10. Увеличили до 11. положили. В мэпе объект со счётчиком 11. А надо 12. Вот это - «сложный» случай, нужна синхронизация. ConcurrentHashMap гарантирует лишь целостность своей структуры, когда несколько потоков пишут/читают.

1. Вообще как сделано: у класса сервлета поле с мапой объявлено как Map<String, MyClass> myclassMap (т.е. interface вместо implementation). В методе же обновления создается implementation мапа (HashMap / ConcurrentHashMap etc.), заполняется, а потом просто myclassMap = та-самая-созданная-и-заполненная-мапа.

Если для приложения нормально, что оно может видеть старые данные вместе с новыми - то можно во время обновления просто делать put()/remove(). Если нельзя - то делаешь как сейчас, но ссылку на мапу - volatile.

2. В сервлете есть таймер. По таймеру выполняется дамп собранных данных в БД. Как тут с синхронизацией? Нужны какие-то пляски? Или таймер всегда будет одним потоком выполняться?

Таймер то будет в одном потоке, но следует учитывать, что пока работает поток из таймера и обновляет мэпу, второй поток может обрабатывать запрос от пользователя.

Ох, как же все сложно-то! Я еще не надоел? :)

Конечно. Два потока взяли объект со счётчиком 10. Увеличили до 11. положили. В мэпе объект со счётчиком 11. А надо 12. Вот это - «сложный» случай, нужна синхронизация. ConcurrentHashMap гарантирует лишь целостность своей структуры, когда несколько потоков пишут/читают.

Сервлет получает запрос и в зависимости от запроса выполняет методы. Например, обрабатывает запрос методом process(). Так если я сделаю даже какой-то блок synchronized в этом методе, то получается, приди сервлету 100 запросов, они все дойдут до этого блока, а там пойдут по одному выполняться?

Сервлет получает запрос и в зависимости от запроса выполняет методы. Например, обрабатывает запрос методом process(). Так если я сделаю даже какой-то блок synchronized в этом методе, то получается, приди сервлету 100 запросов, они все дойдут до этого блока, а там пойдут по одному выполняться?

Да. В этом и смысл synchronized. Но тут есть ньюанс - а надо ли, чтобы все потоки последовательно выполнялись? Если есть потоки, которые только читают данные (внутри метода есть if, например), а другие - модифицируют, и не так часто - то можно вместо synchronized использовать read-write lock. С ним потоки, читающие данные, будут выполняться параллельно. Но если придёт поток, пишущий - все будут ждать, пока он закончит.

Ох, как же все сложно-то!

Чтобы не было сложно, достаточно один раз прочитать нормальную книжку по теме. Причём не обязательно по джаве, это везде всё одинаковое, различаются только доступные средства синхронизации.

Читаю про volatile:

There is a global ordering on the reads and writes to a volatile variable. This implies that every thread accessing a volatile field will read its current value before continuing, instead of (potentially) using a cached value.

Т.е. запись/чтение в/из volatile мапы будет последовательной?

-because accessing a volatile variable never holds a lock, it is not suitable for cases where we want to read-update-write as an atomic operation (unless we're prepared to «miss an update»);

У меня получается как раз read-update-write - получается, в моем случае не очень-то подходит?

-a volatile variable that is an object reference may be null (because you're effectively synchronizing on the reference, not the actual object).

Получается, что синхронизация происходит только для ссылки? Т.е. в моем случае, ссылки на объект ConcurrentHashMap? А на запись/чтение в эту мапу volatile никак не повлияет - повлияет лишь то, что мапа явлется Concurrent?

Ну и вообще, эффективно ли это все будет? Может проще, чтобы была небольшая погрешность в редких случаях, вместо всей этой путаницы и снижении производительности?

Т.е. запись/чтение в/из volatile мапы будет последовательной?

Нет, это относится только к записи/чтения ссылки на эту мэпу.

Получается, что синхронизация происходит только для ссылки? Т.е. в моем случае, ссылки на объект ConcurrentHashMap? А на запись/чтение в эту мапу volatile никак не повлияет - повлияет лишь то, что мапа явлется Concurrent?

Именно для этого я её и предложил - чтобы после обновления мэпы - когда ссылке присваивается новый объект - это изменение видели все потоки.

Ну и вообще, эффективно ли это все будет? Может проще, чтобы была небольшая погрешность в редких случаях, вместо всей этой путаницы и снижении производительности?

От ситуации зависит. Если не критично - то можно и не синхронизировать. А снижение производительности надо сперва доказать в профайлере. Если это действительно будет узкое место - есть множество способов, как повысить производительность, не ставя под сомнение целостность данных.

От ситуации зависит. Если не критично - то можно и не синхронизировать. А снижение производительности надо сперва доказать в профайлере. Если это действительно будет узкое место - есть множество способов, как повысить производительность, не ставя под сомнение целостность данных.

Ну, во всяком случае, от всей это синхронизации ничего не поломается - в худшем случае только работать медленнее будет, да?

Чтобы не было сложно, достаточно один раз прочитать нормальную книжку по теме. Причём не обязательно по джаве, это везде всё одинаковое, различаются только доступные средства синхронизации.

А какую книжку почитать?

Советую на досуге почитать http://jcip.net/, там всё про многопоточность, синхронизацию, гонки и куча рецептов и примеров, как лучше делать.

Сервлеты являются потоконебезопасными классами, и в этой книге или где-то в другом месте прочитал, что лучше все потокоопасные операции выносить в потокобезопасный класс.

Например, в данном случае может имеет смысл сделать отдельный класс-синглтон, внутрь засунуть мапу, а всю бизнес-логику сделать в качестве syncronized-методов этого синглтона?

А в чем будет разница? Почему сам сервлет не сделать потокобезопасным?

Спасибо за книгу!

Ну, во всяком случае, от всей это синхронизации ничего не поломается - в худшем случае только работать медленнее будет, да?

Да.

А какую книжку почитать?

Как уже посоветовали, JCIP.

Большое спасибо!!!

Сервлет можно сделать потокобезопасный.

В чём разница? Я конечно могу вспомнить про MVC и другие красивые слова, но если проще, нагляднее и удобнее всё держать в сервлете, то почему бы и нет.

Ещё могу посоветовать книгу а-ля справочник http://java.sun.com/docs/books/effective/, в которой собраны советы, техники и много толковых вещей.

Effective Java как раз сейчас читаю, отличная книга!

У томката пул есть.. он их не освобождает его без надобности... переиспользовать готовые объекты выгодней чем освобождать и создавать новые а тем более потоки. Если у вас сервер при большом количестве запросов падает с нехваткой памяти, я могу вам в первую очередь посоветовать обзавестись памятью ;)

Не, теперь уже все ок, проблему решили.

А еще такой вопрос: сервлет сам по себе будет singleton'ом (если в коде нигде он не создается, а создается только при запуске приложения - метод init()) ?

Да.

From the servlet 2.3 specification:

For a servlet not hosted in a distributed environment (the default), the servlet container must use only one instance per servlet declaration. However, for a servlet implementing the SingleThreadModel interface, the servlet container may instantiate multiple instances to handle a heavy request load and serialize requests to a particular instance. In the case where a servlet was deployed as part of an application marked in the deployment descriptor as distributable, a container may have only one instance per servlet declaration per virtual machine (VM). However, if the servlet in a distributable application implements the SingleThreadModel interface, the container may instantiate multiple instances of that servlet in each VM of the container.