[Geoserver-devel] Not hard to lock out admin/rest when using FileSystemResource + in memory lock (aka, the default setup)

GeoServer GeoServer Developer
1

Hi,
I was looking at a jstack output provided by a colleague of mine that
managed to get himself locked out of the configuration UI and… well…
from what I can see, it’s not hard at all to get the admin locked out of
the UI, and/or not being able to modify any of it, at least for some time.

Looking at the FileSystemResource implementation we have:

@Override
public InputStream in() {
File actualFile = file();
if (!actualFile.exists()) {
throw new IllegalStateException("File not found " + actualFile);
}
final Lock lock = lock();
try {
return new FileInputStream(file) {
@Override
public void close() throws IOException {
super.close();
lock.release();
}
};
} catch (FileNotFoundException e) {
lock.release();
throw new IllegalStateException("File not found " + actualFile, e);
}
}

So, if the input stream there is not closed, then the lock is not going to be released,
not until garbage collection is triggered.

How common is to grab a in() and not close it? Common, I believe I’ve fixed at least
a couple such occurrences in the last months, just stumbling by accident on them.
I made a quick check, Eclipse can see 35 calls of that in() method,
spotted a few hwere it’s not closed, which means, has the lock that’s not released until
the finalizer calls close(),

One is is logging logging (which is fixed on master btw, but not in 2.8.x):
https://github.com/geoserver/geoserver/blob/2.8.x/src/main/src/main/java/org/geoserver/logging/LoggingUtils.java#L180

One parsing styles, because the reader is turned eventually into an InputSource, which does not even
have a close method:
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/catalog/StyleHandler.java#L192

There might be others in code that I have not available in Eclipse, yet, it’s a worrisome since
a common resource management error can get to a short lived lockdown (how short, depends on gc
and finalization, which are not predictable).

When that happens however all GUI and REST access goes bye bye, because those calls are grabbing
the global configuration lock (our default catalog facade is not thread safe, remember?), then they get
stuck on the resource own lock, and until that gets released, the global config lock is also kept, meaning
every other admin type call is blocked too…

Before the resource refactor that was not much a big of a deal, since there was only the global config lock,
and stream mis-management was not going to cause big issues (well, at least not on linux).

It’s also making me wonder a bit about the resource level locks… given that we have already the
global resource lock, which is not going away until we can rewrite the default catalog facade (and maybe more,
what we’d really need it transactional memory…), is it worth having also a resource level lock for stand-alone GeoServers?

I have the impression that if the global config lock is not disabled (there is a sysvar to do that) we’d
better default on a null lock provider, instead of an in memory one like today?
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/config/LockProviderInitializer.java#L53

The current two levels of lock are redundant for a stand alone server, and are actually causing trouble when
resource input streams are not closed properly…

Opinions? I believe we need at a solid but quick fix for this one, the stable series it too prone to lock itself up
at the moment, and possibly a plan for a better fix in the future.

Cheers
Andrea

···

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

2

Good hunting, we can of course make the null lock provider the default (and should probably due so for the 2.9 release). Long term I would like to fix all the references to in()

···

On 11 April 2016 at 11:09, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
I was looking at a jstack output provided by a colleague of mine that
managed to get himself locked out of the configuration UI and… well…
from what I can see, it’s not hard at all to get the admin locked out of
the UI, and/or not being able to modify any of it, at least for some time.

Looking at the FileSystemResource implementation we have:

@Override
public InputStream in() {
File actualFile = file();
if (!actualFile.exists()) {
throw new IllegalStateException("File not found " + actualFile);
}
final Lock lock = lock();
try {
return new FileInputStream(file) {
@Override
public void close() throws IOException {
super.close();
lock.release();
}
};
} catch (FileNotFoundException e) {
lock.release();
throw new IllegalStateException("File not found " + actualFile, e);
}
}

So, if the input stream there is not closed, then the lock is not going to be released,
not until garbage collection is triggered.

How common is to grab a in() and not close it? Common, I believe I’ve fixed at least
a couple such occurrences in the last months, just stumbling by accident on them.
I made a quick check, Eclipse can see 35 calls of that in() method,
spotted a few hwere it’s not closed, which means, has the lock that’s not released until
the finalizer calls close(),

One is is logging logging (which is fixed on master btw, but not in 2.8.x):
https://github.com/geoserver/geoserver/blob/2.8.x/src/main/src/main/java/org/geoserver/logging/LoggingUtils.java#L180

One parsing styles, because the reader is turned eventually into an InputSource, which does not even
have a close method:
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/catalog/StyleHandler.java#L192

There might be others in code that I have not available in Eclipse, yet, it’s a worrisome since
a common resource management error can get to a short lived lockdown (how short, depends on gc
and finalization, which are not predictable).

When that happens however all GUI and REST access goes bye bye, because those calls are grabbing
the global configuration lock (our default catalog facade is not thread safe, remember?), then they get
stuck on the resource own lock, and until that gets released, the global config lock is also kept, meaning
every other admin type call is blocked too…

Before the resource refactor that was not much a big of a deal, since there was only the global config lock,
and stream mis-management was not going to cause big issues (well, at least not on linux).

It’s also making me wonder a bit about the resource level locks… given that we have already the
global resource lock, which is not going away until we can rewrite the default catalog facade (and maybe more,
what we’d really need it transactional memory…), is it worth having also a resource level lock for stand-alone GeoServers?

I have the impression that if the global config lock is not disabled (there is a sysvar to do that) we’d
better default on a null lock provider, instead of an in memory one like today?
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/config/LockProviderInitializer.java#L53

The current two levels of lock are redundant for a stand alone server, and are actually causing trouble when
resource input streams are not closed properly…

Opinions? I believe we need at a solid but quick fix for this one, the stable series it too prone to lock itself up
at the moment, and possibly a plan for a better fix in the future.

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–
Jody Garnett

3

Hi Jody,
I’ve made a couple of pull requests for master and 2.8.x, changing the default lock provider, fixing the input
stream leaks, and since not closing streams can now cause temporary server lockout, I’ve also added
jdbc connection style leak warning and reporting, with a system variable that can be setup to locate where
the leak is coming from.

https://github.com/geoserver/geoserver/pull/1563 (master)

https://github.com/geoserver/geoserver/pull/1564 (2.8.x)

Cheers
Andrea

···

On Tue, Apr 12, 2016 at 7:55 AM, Jody Garnett <jody.garnett@anonymised.com.403…> wrote:

Good hunting, we can of course make the null lock provider the default (and should probably due so for the 2.9 release). Long term I would like to fix all the references to in()

–

–
Jody Garnett

On 11 April 2016 at 11:09, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
I was looking at a jstack output provided by a colleague of mine that
managed to get himself locked out of the configuration UI and… well…
from what I can see, it’s not hard at all to get the admin locked out of
the UI, and/or not being able to modify any of it, at least for some time.

Looking at the FileSystemResource implementation we have:

@Override
public InputStream in() {
File actualFile = file();
if (!actualFile.exists()) {
throw new IllegalStateException("File not found " + actualFile);
}
final Lock lock = lock();
try {
return new FileInputStream(file) {
@Override
public void close() throws IOException {
super.close();
lock.release();
}
};
} catch (FileNotFoundException e) {
lock.release();
throw new IllegalStateException("File not found " + actualFile, e);
}
}

So, if the input stream there is not closed, then the lock is not going to be released,
not until garbage collection is triggered.

How common is to grab a in() and not close it? Common, I believe I’ve fixed at least
a couple such occurrences in the last months, just stumbling by accident on them.
I made a quick check, Eclipse can see 35 calls of that in() method,
spotted a few hwere it’s not closed, which means, has the lock that’s not released until
the finalizer calls close(),

One is is logging logging (which is fixed on master btw, but not in 2.8.x):
https://github.com/geoserver/geoserver/blob/2.8.x/src/main/src/main/java/org/geoserver/logging/LoggingUtils.java#L180

One parsing styles, because the reader is turned eventually into an InputSource, which does not even
have a close method:
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/catalog/StyleHandler.java#L192

There might be others in code that I have not available in Eclipse, yet, it’s a worrisome since
a common resource management error can get to a short lived lockdown (how short, depends on gc
and finalization, which are not predictable).

When that happens however all GUI and REST access goes bye bye, because those calls are grabbing
the global configuration lock (our default catalog facade is not thread safe, remember?), then they get
stuck on the resource own lock, and until that gets released, the global config lock is also kept, meaning
every other admin type call is blocked too…

Before the resource refactor that was not much a big of a deal, since there was only the global config lock,
and stream mis-management was not going to cause big issues (well, at least not on linux).

It’s also making me wonder a bit about the resource level locks… given that we have already the
global resource lock, which is not going away until we can rewrite the default catalog facade (and maybe more,
what we’d really need it transactional memory…), is it worth having also a resource level lock for stand-alone GeoServers?

I have the impression that if the global config lock is not disabled (there is a sysvar to do that) we’d
better default on a null lock provider, instead of an in memory one like today?
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/config/LockProviderInitializer.java#L53

The current two levels of lock are redundant for a stand alone server, and are actually causing trouble when
resource input streams are not closed properly…

Opinions? I believe we need at a solid but quick fix for this one, the stable series it too prone to lock itself up
at the moment, and possibly a plan for a better fix in the future.

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532

Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

4

Thanks Andrea, especially for the leak warning and reporting (seems to be -Dgs.lock.trace=true).

···

On 12 April 2016 at 03:50, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi Jody,
I’ve made a couple of pull requests for master and 2.8.x, changing the default lock provider, fixing the input
stream leaks, and since not closing streams can now cause temporary server lockout, I’ve also added
jdbc connection style leak warning and reporting, with a system variable that can be setup to locate where
the leak is coming from.

https://github.com/geoserver/geoserver/pull/1563 (master)

https://github.com/geoserver/geoserver/pull/1564 (2.8.x)

Cheers

Andrea

–
Jody Garnett

On Tue, Apr 12, 2016 at 7:55 AM, Jody Garnett <jody.garnett@anonymised.com.> wrote:

Good hunting, we can of course make the null lock provider the default (and should probably due so for the 2.9 release). Long term I would like to fix all the references to in()

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

–
Jody Garnett

On 11 April 2016 at 11:09, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
I was looking at a jstack output provided by a colleague of mine that
managed to get himself locked out of the configuration UI and… well…
from what I can see, it’s not hard at all to get the admin locked out of
the UI, and/or not being able to modify any of it, at least for some time.

Looking at the FileSystemResource implementation we have:

@Override
public InputStream in() {
File actualFile = file();
if (!actualFile.exists()) {
throw new IllegalStateException("File not found " + actualFile);
}
final Lock lock = lock();
try {
return new FileInputStream(file) {
@Override
public void close() throws IOException {
super.close();
lock.release();
}
};
} catch (FileNotFoundException e) {
lock.release();
throw new IllegalStateException("File not found " + actualFile, e);
}
}

So, if the input stream there is not closed, then the lock is not going to be released,
not until garbage collection is triggered.

How common is to grab a in() and not close it? Common, I believe I’ve fixed at least
a couple such occurrences in the last months, just stumbling by accident on them.
I made a quick check, Eclipse can see 35 calls of that in() method,
spotted a few hwere it’s not closed, which means, has the lock that’s not released until
the finalizer calls close(),

One is is logging logging (which is fixed on master btw, but not in 2.8.x):
https://github.com/geoserver/geoserver/blob/2.8.x/src/main/src/main/java/org/geoserver/logging/LoggingUtils.java#L180

One parsing styles, because the reader is turned eventually into an InputSource, which does not even
have a close method:
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/catalog/StyleHandler.java#L192

There might be others in code that I have not available in Eclipse, yet, it’s a worrisome since
a common resource management error can get to a short lived lockdown (how short, depends on gc
and finalization, which are not predictable).

When that happens however all GUI and REST access goes bye bye, because those calls are grabbing
the global configuration lock (our default catalog facade is not thread safe, remember?), then they get
stuck on the resource own lock, and until that gets released, the global config lock is also kept, meaning
every other admin type call is blocked too…

Before the resource refactor that was not much a big of a deal, since there was only the global config lock,
and stream mis-management was not going to cause big issues (well, at least not on linux).

It’s also making me wonder a bit about the resource level locks… given that we have already the
global resource lock, which is not going away until we can rewrite the default catalog facade (and maybe more,
what we’d really need it transactional memory…), is it worth having also a resource level lock for stand-alone GeoServers?

I have the impression that if the global config lock is not disabled (there is a sysvar to do that) we’d
better default on a null lock provider, instead of an in memory one like today?
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/config/LockProviderInitializer.java#L53

The current two levels of lock are redundant for a stand alone server, and are actually causing trouble when
resource input streams are not closed properly…

Opinions? I believe we need at a solid but quick fix for this one, the stable series it too prone to lock itself up
at the moment, and possibly a plan for a better fix in the future.

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532

Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

5

Hi Jody,
wanted to check if you want to review them (seems like you had a quick look already)
or I should just go ahead and merge?

Cheers
Andrea

PS: in case of unclosed streams the logged warning states which sysvar should be set (sparing one
to have to look for it, which is always annoying for its sibling gt.jdbc.trace one)

···

On Tue, Apr 12, 2016 at 6:07 PM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Thanks Andrea, especially for the leak warning and reporting (seems to be -Dgs.lock.trace=true).

–

–
Jody Garnett

On 12 April 2016 at 03:50, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi Jody,
I’ve made a couple of pull requests for master and 2.8.x, changing the default lock provider, fixing the input
stream leaks, and since not closing streams can now cause temporary server lockout, I’ve also added
jdbc connection style leak warning and reporting, with a system variable that can be setup to locate where
the leak is coming from.

https://github.com/geoserver/geoserver/pull/1563 (master)

https://github.com/geoserver/geoserver/pull/1564 (2.8.x)

Cheers

Andrea

On Tue, Apr 12, 2016 at 7:55 AM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Good hunting, we can of course make the null lock provider the default (and should probably due so for the 2.9 release). Long term I would like to fix all the references to in()

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

–
Jody Garnett

On 11 April 2016 at 11:09, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
I was looking at a jstack output provided by a colleague of mine that
managed to get himself locked out of the configuration UI and… well…
from what I can see, it’s not hard at all to get the admin locked out of
the UI, and/or not being able to modify any of it, at least for some time.

Looking at the FileSystemResource implementation we have:

@Override
public InputStream in() {
File actualFile = file();
if (!actualFile.exists()) {
throw new IllegalStateException("File not found " + actualFile);
}
final Lock lock = lock();
try {
return new FileInputStream(file) {
@Override
public void close() throws IOException {
super.close();
lock.release();
}
};
} catch (FileNotFoundException e) {
lock.release();
throw new IllegalStateException("File not found " + actualFile, e);
}
}

So, if the input stream there is not closed, then the lock is not going to be released,
not until garbage collection is triggered.

How common is to grab a in() and not close it? Common, I believe I’ve fixed at least
a couple such occurrences in the last months, just stumbling by accident on them.
I made a quick check, Eclipse can see 35 calls of that in() method,
spotted a few hwere it’s not closed, which means, has the lock that’s not released until
the finalizer calls close(),

One is is logging logging (which is fixed on master btw, but not in 2.8.x):
https://github.com/geoserver/geoserver/blob/2.8.x/src/main/src/main/java/org/geoserver/logging/LoggingUtils.java#L180

One parsing styles, because the reader is turned eventually into an InputSource, which does not even
have a close method:
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/catalog/StyleHandler.java#L192

There might be others in code that I have not available in Eclipse, yet, it’s a worrisome since
a common resource management error can get to a short lived lockdown (how short, depends on gc
and finalization, which are not predictable).

When that happens however all GUI and REST access goes bye bye, because those calls are grabbing
the global configuration lock (our default catalog facade is not thread safe, remember?), then they get
stuck on the resource own lock, and until that gets released, the global config lock is also kept, meaning
every other admin type call is blocked too…

Before the resource refactor that was not much a big of a deal, since there was only the global config lock,
and stream mis-management was not going to cause big issues (well, at least not on linux).

It’s also making me wonder a bit about the resource level locks… given that we have already the
global resource lock, which is not going away until we can rewrite the default catalog facade (and maybe more,
what we’d really need it transactional memory…), is it worth having also a resource level lock for stand-alone GeoServers?

I have the impression that if the global config lock is not disabled (there is a sysvar to do that) we’d
better default on a null lock provider, instead of an in memory one like today?
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/config/LockProviderInitializer.java#L53

The current two levels of lock are redundant for a stand alone server, and are actually causing trouble when
resource input streams are not closed properly…

Opinions? I believe we need at a solid but quick fix for this one, the stable series it too prone to lock itself up
at the moment, and possibly a plan for a better fix in the future.

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532

Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.