You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "nickva (via GitHub)" <gi...@apache.org> on 2023/08/23 20:55:42 UTC
[GitHub] [couchdb] nickva opened a new pull request, #4736: Stop client process and clean up if client disconnects
nickva opened a new pull request, #4736:
URL: https://github.com/apache/couchdb/pull/4736
Previously, when processing long running streaming requests, such as _find with a highly selective selector, when no rows are emitted for a while, the client could disconnect but the request process, and all the associated workers on remote nodes would continue running, consuming server resources.
We already handle remote (RPC) process cleanup if the coordinator crashes, we just need a way to kill that coordinator if the connection is closed in the case when no data may be emitted for a long time. This is what the current commit accomplishes.
It turns out there is no simple way to detect passive mode socket disconnects in current versions of Erlang/OTP. The socket at the kernel level may be closed (in close_wait state), however `inet:info/1` will continue reporting it as `connected`. The only ways to obtain an accurate connection state is to try to write, read, or query the TCP info state.
Here we attempt to query the state with tcp_info. Unfortunately, not all versions of supported OTP platforms have this option, so it probably never became an officially supported inet socket option, so we use `raw` socket option for it. However, it turns out most of CouchDB supported platforms do have that option. The only platform this is not working currently is Windows. In that case, or future platform cases we default to the previous behavior, assuming the socket is still open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#issuecomment-1690978405
Wondering about the performance cost of `inet:getopts/2` with `tcp_info` raw option. It looks to be about 1-2 microseconds on both Linux and MacOS:
Mac
```
> element(1, timer:tc(fun() -> lists:foreach(fun(_) -> inet:getopts(#Port<0.33>, [{raw, 6, 262, 1}]) end, lists:seq(1, 10000)) end)) / 10000.
1.4706
```
Linux
```
element(1, timer:tc(fun() -> lists:foreach(fun(_) -> inet:getopts(#Port<0.5>, [{raw, 6, 11, 1}]) end, lists:seq(1, 10000)) end)) / 10000.
1.7163
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1305907711
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,113 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add jitter to avoid a stampede in case of a larger number of concurrent connections
+ MSec + rand:uniform(MSec).
Review Comment:
Updated with a configurable jitter value defaulting to 15000 msec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#issuecomment-1690634179
I like this, tidier than https://github.com/apache/couchdb/pull/4729 and the tcp decoding bits aren't too bad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303560627
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
Review Comment:
Good point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303558016
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
+ %% IPPROTO_TCP = 6
+ %%
+ %% include/netinet/tcp.h
+ %% #define TCP_INFO 11
+ %%
+ {raw, 6, 11, 1};
+tcp_info_opt({unix, darwin}) ->
+ %% include/netinet/in.h
+ %% #define IPPROTO_TCP 6
+ %%
+ %% netinet/tcp.h
+ %% #define TCP_CONNECTION_INFO 0x106
+ %%
+ {raw, 6, 16#106, 1};
+tcp_info_opt({unix, freebsd}) ->
+ %% sys/netinet/in.h
+ %% #define IPPROTO_TCP 6 /* tcp */
+ %%
+ %% sys/netinet/tcp.h
+ %% #define TCP_INFO 32 /* retrieve tcp_info structure */
+ %%
+ {raw, 6, 32, 1};
+tcp_info_opt({_, _}) ->
Review Comment:
aye, the fallback is fine. It's not a blocker to have a clause for Windows imo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303571095
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
Review Comment:
updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1305794215
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
+ %% IPPROTO_TCP = 6
+ %%
+ %% include/netinet/tcp.h
+ %% #define TCP_INFO 11
+ %%
+ {raw, 6, 11, 1};
+tcp_info_opt({unix, darwin}) ->
+ %% include/netinet/in.h
+ %% #define IPPROTO_TCP 6
+ %%
+ %% netinet/tcp.h
+ %% #define TCP_CONNECTION_INFO 0x106
+ %%
+ {raw, 6, 16#106, 1};
+tcp_info_opt({unix, freebsd}) ->
+ %% sys/netinet/in.h
+ %% #define IPPROTO_TCP 6 /* tcp */
+ %%
+ %% sys/netinet/tcp.h
+ %% #define TCP_INFO 32 /* retrieve tcp_info structure */
+ %%
+ {raw, 6, 32, 1};
+tcp_info_opt({_, _}) ->
Review Comment:
we concluded elsewhere that there isn't a way to do this on Windows with the functions erlang/otp currently exposes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva merged PR #4736:
URL: https://github.com/apache/couchdb/pull/4736
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#issuecomment-1692072177
Testing with a local TLS listening socket:
With a patch to block _all_docs:
```diff
diff --git a/src/fabric/src/fabric_view_all_docs.erl b/src/fabric/src/fabric_view_all_docs.erl
index a6786bff7..b0bbdaf5a 100644
--- a/src/fabric/src/fabric_view_all_docs.erl
+++ b/src/fabric/src/fabric_view_all_docs.erl
@@ -263,6 +263,8 @@ handle_message(#view_row{} = Row, {Worker, From}, State) ->
Rows = merge_row(Dir, Row#view_row{worker = {Worker, From}}, Rows0),
Counters1 = fabric_dict:update_counter(Worker, 1, Counters0),
State1 = State#collector{rows = Rows, counters = Counters1},
+ couch_log:error(" +++sleeping before sending row++++ ~p:~p@~B~p", [?MODULE, ?FUNCTION_NAME, ?LINE, Row]),
+ timer:sleep(60000),
fabric_view:maybe_send_row(State1);
```
I see the expected log line:
```
[warning] 2023-08-24T16:40:44.677817Z node1@127.0.0.1 <0.2203.0> -------- client socket {ssl,{sslsocket,{gen_tcp,#Port<0.18>,tls_connection,[{option_tracker,<0.636.0>},{session_tickets_tracker,disabled},{session_id_tracker,<0.637.0>}]},[<0.2191.0>,<0.2190.0>]}} disconnected, terminating <0.2061.0>
```
That seems too verbose so after discussing with @rnewson we figured we should remove the log line altogether.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#issuecomment-1690906135
Ran the [full CI](https://ci-couchdb.apache.org/blue/organizations/jenkins/jenkins-cm1%2FFullPlatformMatrix/detail/jenkins-monitor-client-socket-for-disconnections/1/pipeline/79) test to check exercise the code on a variety of platforms in:
<img width="549" alt="Screen Shot 2023-08-23 at 10 38 53 PM" src="https://github.com/apache/couchdb/assets/211822/873f0bd3-5971-4651-918e-4f2fc0536574">
(.deb package builds fail for an unrelated reason but all the main CI steps pass)
The combination of os-es include `ubuntu`, `debian`, `centos`, `macos`, `freebsd`. Architectures include: `x86_64`, `ppc64le`, `s390x`, `arm64`. All tests pass on all currently available platforms. On Windows I tested a script manually to show that inet:getopts returns `{ok, []}` so we'd fall back to a default of `socket not closed`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303542422
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
+ %% IPPROTO_TCP = 6
+ %%
+ %% include/netinet/tcp.h
+ %% #define TCP_INFO 11
+ %%
+ {raw, 6, 11, 1};
+tcp_info_opt({unix, darwin}) ->
+ %% include/netinet/in.h
+ %% #define IPPROTO_TCP 6
+ %%
+ %% netinet/tcp.h
+ %% #define TCP_CONNECTION_INFO 0x106
+ %%
+ {raw, 6, 16#106, 1};
+tcp_info_opt({unix, freebsd}) ->
+ %% sys/netinet/in.h
+ %% #define IPPROTO_TCP 6 /* tcp */
+ %%
+ %% sys/netinet/tcp.h
+ %% #define TCP_INFO 32 /* retrieve tcp_info structure */
+ %%
+ {raw, 6, 32, 1};
+tcp_info_opt({_, _}) ->
Review Comment:
and we owe a clause for Windows (alas)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303541545
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
Review Comment:
ah, good, `mochiweb_socket:getopts` handles both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303541026
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
Review Comment:
we maybe need an
```
is_mochiweb_socket_closed({ssl, Sock}) ->
is_mochiweb_socket_closed(Sock);
```
clause for couchdb's configured for native TLS?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1305820713
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,113 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add jitter to avoid a stampede in case of a larger number of concurrent connections
+ MSec + rand:uniform(MSec).
Review Comment:
What if we made it a smaller percentage of jitter (+/-20%) centered around the value, to avoid having another config option.
```erlang
MSec * 0.80 + rand:uniform(MSec * 0.4)
```
Or a separate value completely would still be better?:
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,113 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add jitter to avoid a stampede in case of a larger number of concurrent connections
+ MSec + rand:uniform(MSec).
Review Comment:
What if we made it a smaller percentage of jitter (+/-20%) centered around the value, to avoid having another config option.
```erlang
MSec * 0.80 + rand:uniform(MSec * 0.4)
```
Or a separate value completely would still be better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1305792655
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,113 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add jitter to avoid a stampede in case of a larger number of concurrent connections
+ MSec + rand:uniform(MSec).
Review Comment:
might want the amount of jitter as a config item also. 30s-60s as a spread seems fine but becomes less reasonable if an admin configures a different value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#issuecomment-1693474756
@rnewson Good idea about trying to make a PR for mochiweb. I think we could make a case for it to fit in either mochiweb_socket or mochiweb_request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "davisp (via GitHub)" <gi...@apache.org>.
davisp commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303557136
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
Review Comment:
Generally speaking, the `include` directory isn't used as part of the header name. Like you did in most other places except here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#issuecomment-1692131403
The previous warning log line was replaced with a stats bump:
```
{[couchdb, httpd, abandoned_streaming_requests], [
{type, counter},
{desc, <<"number of abandoned streaming requests">>}
]}.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4736: Stop client process and clean up if client disconnects
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4736:
URL: https://github.com/apache/couchdb/pull/4736#discussion_r1303545622
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
+ %% IPPROTO_TCP = 6
+ %%
+ %% include/netinet/tcp.h
+ %% #define TCP_INFO 11
+ %%
+ {raw, 6, 11, 1};
+tcp_info_opt({unix, darwin}) ->
+ %% include/netinet/in.h
+ %% #define IPPROTO_TCP 6
+ %%
+ %% netinet/tcp.h
+ %% #define TCP_CONNECTION_INFO 0x106
+ %%
+ {raw, 6, 16#106, 1};
+tcp_info_opt({unix, freebsd}) ->
+ %% sys/netinet/in.h
+ %% #define IPPROTO_TCP 6 /* tcp */
+ %%
+ %% sys/netinet/tcp.h
+ %% #define TCP_INFO 32 /* retrieve tcp_info structure */
+ %%
+ {raw, 6, 32, 1};
+tcp_info_opt({_, _}) ->
Review Comment:
👍 there may be some incantation for windows but I couldn't figure it out. For unsupported (yet) we'd just default to the already existing behavior.
##########
src/chttpd/src/chttpd_util.erl:
##########
@@ -110,3 +118,125 @@ get_db_info(DbName) ->
catch
_Tag:Error -> {error, Error}
end.
+
+mochiweb_socket_set(Sock) ->
+ put(?MOCHIWEB_SOCKET, Sock).
+
+mochiweb_socket_get() ->
+ get(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_clean() ->
+ erase(?MOCHIWEB_SOCKET).
+
+mochiweb_socket_check_msec() ->
+ MSec = config:get_integer("chttpd", "disconnect_check_msec", ?DISCONNECT_CHECK_MSEC),
+ % Add some jitter to avoid a stampede in case of a larger number of concurrent connection
+ MSec + rand:uniform(MSec).
+
+stop_client_process_if_disconnected(Pid, Sock) ->
+ case is_mochiweb_socket_closed(Sock) of
+ true ->
+ couch_log:warning("client socket ~p disconnected, terminating ~p", [Sock, Pid]),
+ exit(Pid, {shutdown, client_disconnected});
+ false ->
+ ok
+ end.
+
+is_mochiweb_socket_closed(undefined) ->
+ false;
+is_mochiweb_socket_closed(Sock) ->
+ OsType = os:type(),
+ case tcp_info_opt(OsType) of
+ {raw, _, _, _} = InfoOpt ->
+ case mochiweb_socket:getopts(Sock, [InfoOpt]) of
+ {ok, [{raw, _, _, <<State:8/native, _/binary>>}]} ->
+ tcp_is_closed(State, OsType);
+ {ok, []} ->
+ false;
+ {error, einval} ->
+ % Already cleaned up
+ true;
+ {error, _} ->
+ false
+ end;
+ undefined ->
+ false
+ end.
+
+% All OS-es have the tcpi_state (uint8) as first member of tcp_info struct
+
+tcp_info_opt({unix, linux}) ->
+ %% include/netinet/in.h
+ %% IPPROTO_TCP = 6
+ %%
+ %% include/netinet/tcp.h
+ %% #define TCP_INFO 11
+ %%
+ {raw, 6, 11, 1};
+tcp_info_opt({unix, darwin}) ->
+ %% include/netinet/in.h
+ %% #define IPPROTO_TCP 6
+ %%
+ %% netinet/tcp.h
+ %% #define TCP_CONNECTION_INFO 0x106
+ %%
+ {raw, 6, 16#106, 1};
+tcp_info_opt({unix, freebsd}) ->
+ %% sys/netinet/in.h
+ %% #define IPPROTO_TCP 6 /* tcp */
+ %%
+ %% sys/netinet/tcp.h
+ %% #define TCP_INFO 32 /* retrieve tcp_info structure */
+ %%
+ {raw, 6, 32, 1};
+tcp_info_opt({_, _}) ->
Review Comment:
👍 there may be some incantation for windows but I couldn't figure it out. For unsupported (yet) we'd just default to the already existing behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org