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

Close idle GELF HTTP connections after a timeout #3315

Merged
merged 4 commits into from Jan 9, 2017

Conversation

Projects
None yet
2 participants
@bernd
Member

bernd commented Jan 6, 2017

This introduces a new GELF HTTP input option to configure a timeout for idle HTTP connections.

Some clients (e.g. browsers) might not close the HTTP connection properly and effectively cause a file descriptor leak in the server. The default for all existing and new GELF HTTP inputs is 60 seconds.

Fixes #3223

Close idle GELF HTTP connections after a timeout
This introduces a new GELF HTTP input option to configure a timeout for
idle HTTP connections.

Some clients (e.g. browsers) might not close the HTTP connection
properly and effectively cause a file descriptor leak in the server. The
default for all existing and new GELF HTTP inputs is 60 seconds.

Fixes #3223

@bernd bernd added this to the 2.2.0 milestone Jan 6, 2017

@@ -184,9 +217,24 @@ public ConfigurationRequest getRequestedConfiguration() {
DEFAULT_MAX_CHUNK_SIZE,
"The maximum HTTP chunk size in bytes (e. g. length of HTTP request body)",
ConfigurationField.Optional.OPTIONAL));
r.addField(new NumberField(CK_IDLE_WRITER_TIMEOUT,

This comment has been minimized.

@joschi

joschi Jan 9, 2017

Contributor

Maybe add NumberField.Attribute.ONLY_POSITIVE here.

This comment has been minimized.

@bernd

bernd Jan 9, 2017

Member

👍

baseChannelHandlers.put("idlestate", new Callable<ChannelHandler>() {
@Override
public ChannelHandler call() throws Exception {
return new IdleStateHandler(timer, 0, idleWriterTimeout, 0, TimeUnit.SECONDS);

This comment has been minimized.

@joschi

joschi Jan 9, 2017

Contributor

Why use an IdleStateHandler instead of WriteTimeoutHandler here if only the write timeout is being used?

This comment has been minimized.

@bernd

bernd Jan 9, 2017

Member

Because I wasn't aware of it. Changed the code to use a ReadTimeoutHandler because I want to close the connection when the client does not write anymore. Got it mixed up. 😉

This comment has been minimized.

@joschi

joschi Jan 9, 2017

Contributor

👍

int maxChunkSize = configuration.intIsSet(CK_MAX_CHUNK_SIZE) ? configuration.getInt(CK_MAX_CHUNK_SIZE) : DEFAULT_MAX_CHUNK_SIZE;
this.maxChunkSize = maxChunkSize <= 0 ? DEFAULT_MAX_CHUNK_SIZE : maxChunkSize;
this.idleWriterTimeout = configuration.intIsSet(CK_MAX_CHUNK_SIZE) ? configuration.getInt(CK_IDLE_WRITER_TIMEOUT, DEFAULT_IDLE_WRITER_TIMEOUT) : DEFAULT_IDLE_WRITER_TIMEOUT;

This comment has been minimized.

@joschi

joschi Jan 9, 2017

Contributor

This checks for the wrong configuration key.

This comment has been minimized.

@bernd

bernd Jan 9, 2017

Member

Oups, good catch! 👍

@joschi joschi self-assigned this Jan 9, 2017

@joschi

joschi approved these changes Jan 9, 2017

@joschi joschi merged commit 3a081ce into master Jan 9, 2017

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1253 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@joschi joschi deleted the issue-3223 branch Jan 9, 2017

@joschi joschi removed the ready-for-review label Jan 9, 2017

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