fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary #10783

liverpool8056 · 2023-05-04T07:22:54Z

Summary

For those sockets created inside the schedule of timer-ng, the sockets must be closed explicitly.
More details, please refer to: #10691

Checklist

The Pull Request has tests
There's an entry in the CHANGELOG
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

[Implement ...]

Issue reference

#10691

CHANGELOG.md

ADD-SP

For the TCP socket, would it be more proper to replace socket:close() with socket:setkeepalive()?

JUST A SUGGESTION.

ms2008 · 2023-05-04T07:58:56Z

kong/plugins/tcp-log/handler.lua

@@ -27,13 +27,15 @@ local function log(premature, conf, message)
  local ok, err = sock:connect(host, port)
  if not ok then
    kong.log.err("failed to connect to ", host, ":", tostring(port), ": ", err)
+    sock:close()


I'm a little curious, is it still need to explicitly call close when an error occurs? Most languages don't seem to need to do that, and it doesn't seem to do so in the official examples I've seen.

A reasonable concern. Please let us know if you have some conclusion about it, @oowl

Fatal errors in cosocket operations always automatically close the current connection (note that, read timeout error is the only error that is not fatal), and if you call close on a closed connection, you will get the "closed" error.

https://github.com/openresty/lua-nginx-module#ngxsockettcp

Even if the socket will be closed when a fatal occurred, I think closing a failed socket explicitly is a good style.

I think closing a failed socket explicitly is a good style.

I have the opposite opinion, I think such code is not clean enough and feels very verbose.

liverpool8056 · 2023-05-04T08:41:07Z

@ADD-SP Yes, I just close them when there is something wrong with the connections.

ADD-SP

I think writing tests for this PR is not worth until we find a proper, right, non-flaky way to write the test.

We can't find a great way to test this fix now. if we just write test to follow the PR review process, it will introduce some bad code in our test set. The final target of the PR review process is for a better code base, writing tests is for that, and refusing bad test code is also for that.

ADD-SP

Duplicate #10783 (review) due to bad network

kong/reports.lua

…cessary For those sockets created inside the schedule of timer-ng, the sockets must be closed explicitly. More details, please refer to: [#10691](#10691)

Co-authored-by: Chrono <chrono_cpp@me.com>

Co-authored-by: Jun Ouyang <ouyangjun1999@gmail.com>

kikito · 2023-05-19T21:33:45Z

CHANGELOG.md

  [#10790](https://github.com/Kong/kong/pull/10790)
+- **loggly & tcp-log & udp-log**: fix a potential issue that could cause socket leaks.


Note that this change was added to the wrong version in the changelog - it should have gone into #unreleased

I revised the changes one by one. Besides trivial changes in punctuation/order: #10783 (Reports: fix a potential issue that could cause socket leaks. and **loggly & tcp-log & udp-log**: fix a potential issue that could cause socket leaks.) was resolved after code freeze and was not backported to 3.3 - its changelog entry was put in the wrong place #

…0909) I revised the changes one by one. Besides trivial changes in punctuation/order: #10783 (Reports: fix a potential issue that could cause socket leaks. and **loggly & tcp-log & udp-log**: fix a potential issue that could cause socket leaks.) was resolved after code freeze and was not backported to 3.3 - its changelog entry was put in the wrong place #

pull-request-size bot added the size/S label May 4, 2023

github-actions bot added plugins/loggly plugins/tcp-log plugins/udp-log labels May 4, 2023

github-actions bot assigned liverpool8056 May 4, 2023

liverpool8056 force-pushed the fix/sockets-release branch from 8e80164 to 73a46d5 Compare May 4, 2023 07:30

github-actions bot added the changelog label May 4, 2023

liverpool8056 requested a review from ADD-SP May 4, 2023 07:30

chronolaw reviewed May 4, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chronolaw reviewed May 4, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ADD-SP reviewed May 4, 2023

View reviewed changes

ms2008 reviewed May 4, 2023

View reviewed changes

liverpool8056 force-pushed the fix/sockets-release branch from 4b6e449 to ecb7276 Compare May 4, 2023 08:48

ADD-SP approved these changes May 8, 2023

View reviewed changes

oowl requested changes May 15, 2023

View reviewed changes

kong/reports.lua Outdated Show resolved Hide resolved

liverpool8056 requested a review from oowl May 15, 2023 11:49

liverpool8056 force-pushed the fix/sockets-release branch from 94ed018 to 600a5a2 Compare May 15, 2023 11:53

oowl approved these changes May 15, 2023

View reviewed changes

liverpool8056 and others added 6 commits May 16, 2023 17:55

fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when ne…

438b9a4

…cessary For those sockets created inside the schedule of timer-ng, the sockets must be closed explicitly. More details, please refer to: [#10691](#10691)

CHANGELOG

1e093be

remove goto

73dec15

Update CHANGELOG.md

4779c6e

Co-authored-by: Chrono <chrono_cpp@me.com>

Update CHANGELOG.md

f52d46c

Co-authored-by: Chrono <chrono_cpp@me.com>

Update kong/reports.lua

1c1ac4e

Co-authored-by: Jun Ouyang <ouyangjun1999@gmail.com>

liverpool8056 force-pushed the fix/sockets-release branch from 600a5a2 to 1c1ac4e Compare May 16, 2023 09:55

windmgc merged commit 9a50b1b into master May 17, 2023

windmgc deleted the fix/sockets-release branch May 17, 2023 03:04

kikito reviewed May 19, 2023

View reviewed changes

kikito mentioned this pull request May 19, 2023

docs(changelog): update changelog for 3.3.0 with the released one #10909

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary #10783

fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary #10783

liverpool8056 commented May 4, 2023

ADD-SP left a comment •

edited

Loading

ms2008 May 4, 2023

liverpool8056 May 4, 2023 •

edited

Loading

ADD-SP May 8, 2023

ms2008 May 9, 2023

liverpool8056 commented May 4, 2023

ADD-SP left a comment •

edited

Loading

ADD-SP left a comment •

edited

Loading

kikito May 19, 2023

		[#10790](https://github.com/Kong/kong/pull/10790)
		- loggly & tcp-log & udp-log: fix a potential issue that could cause socket leaks.

fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary #10783

fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary #10783

Conversation

liverpool8056 commented May 4, 2023

Summary

Checklist

Full changelog

Issue reference

ADD-SP left a comment • edited Loading

Choose a reason for hiding this comment

ms2008 May 4, 2023

Choose a reason for hiding this comment

liverpool8056 May 4, 2023 • edited Loading

Choose a reason for hiding this comment

ADD-SP May 8, 2023

Choose a reason for hiding this comment

ms2008 May 9, 2023

Choose a reason for hiding this comment

liverpool8056 commented May 4, 2023

ADD-SP left a comment • edited Loading

Choose a reason for hiding this comment

ADD-SP left a comment • edited Loading

Choose a reason for hiding this comment

kikito May 19, 2023

Choose a reason for hiding this comment

ADD-SP left a comment •

edited

Loading

liverpool8056 May 4, 2023 •

edited

Loading

ADD-SP left a comment •

edited

Loading

ADD-SP left a comment •

edited

Loading