{"action":"created","comment":{"url":"https://api.github.com/repos/facebook/nailgun/pulls/comments/195533520","pull_request_review_id":128919713,"id":195533520,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDE5NTUzMzUyMA==","diff_hunk":"@@ -63,62 +58,78 @@\n \t * Creates a new NGSessionRunner operating for the specified server, with\n \t * the specified number of threads\n \t * @param server the server to work for\n-\t * @param poolsize the maximum number of idle threads to allow\n+\t * @param maxIdleSessions the maximum number of idle threads to allow\n \t */\n-\tNGSessionPool(NGServer server, int poolsize) {\n+\tNGSessionPool(NGServer server, int maxIdleSessions) {\n \t\tthis.server = server;\n-\t\tthis.poolSize = Math.min(0, poolsize);\n-\t\n-\t\tpool = new NGSession[poolSize];\n-\t\tpoolEntries = 0;\n+\t\tthis.maxIdleSessions = Math.max(0, maxIdleSessions);\n+\n+\t\tidlePool = new LinkedList<>();\n+\t\tworkingPool = new HashSet<>();\n \t}\n \n \t/**\n \t * Returns an NGSession from the pool, or creates one if necessary\n \t * @return an NGSession ready to work\n \t */\n \tNGSession take() {\n-\t\tNGSession result;\n \t\tsynchronized(lock) {\n-\t\t\tif (poolEntries == 0) {\n-\t\t\t\tresult = new NGSession(this, server);\n-\t\t\t\tresult.start();\n-\t\t\t} else {\n-\t\t\t\t--poolEntries;\n-\t\t\t\tresult = pool[poolEntries];\n+\t\t\tif (done) {\n+\t\t\t\tthrow new UnsupportedOperationException(\"NGSession pool is shutting down\");\n \t\t\t}\n+\t\t\tNGSession session = idlePool.poll();\n+\t\t\tif (session == null) {\n+\t\t\t\tsession = new NGSession(this, server);\n+\t\t\t\tsession.start();\n+\t\t\t}\n+\t\t\tworkingPool.add(session);\n+\t\t\treturn session;\n \t\t}\n-\t\treturn (result);\n \t}\n \t\n \t/**\n-\t * Returns an NGSession to the pool.  The pool may choose to shutdown\n-\t * the thread if the pool is full\n+\t * Returns an NGSession to the pool. The pool may choose to shutdown\n+\t * the thread if idle pool is full.\n \t * @param session the NGSession to return to the pool\n \t */\n \tvoid give(NGSession session) {\n-        boolean shutdown = false;\n         synchronized(lock) {\n-    \t\tif (done || poolEntries == poolSize) {\n-                shutdown = true;\n-            } else {\n-                pool[poolEntries] = session;\n-                ++poolEntries;\n-\t    \t}\n+    \t\tif (done) {\n+    \t\t\t// session is already signalled shutdown and removed from all collections\n+    \t\t\treturn;\n+    \t\t}\n+\t\t\tworkingPool.remove(session);\n+\n+      \t\tif (idlePool.size() < maxIdleSessions) {\n+        \t\tidlePool.add(session);\n+        \t\treturn;\n+      \t\t}\n         }\n-        if (shutdown) session.shutdown();\n+       \tsession.shutdown();\n \t}\n \t\n \t/**\n-\t * Shuts down the pool.  Running nails are allowed to finish.\n+\t * Shuts down the pool. The function waits for running nails to finish.\n \t */\n \tvoid shutdown() {\n+\t\tList<NGSession> allSessions;\n \t\tsynchronized(lock) {\n \t\t\tdone = true;\n-\t\t\twhile (poolEntries > 0) {\n-\t\t\t\ttake().shutdown();\n+\t\t\tallSessions = Stream.concat(workingPool.stream(), idlePool.stream()).collect(Collectors.toList());\n+\t\t\tidlePool.clear();\n+\t\t\tworkingPool.clear();\n+\t\t}\n+\t\tfor (NGSession session : allSessions) {\n+\t\t\tsession.shutdown();\n+\t\t}\n+\n+\t\t// wait for all sessions to complete by either returning from waiting state or finishing their nails\n+\t\tfor (NGSession session : allSessions) {\n+\t\t\ttry {\n+\t\t\t\tsession.join();\n+\t\t\t} catch (InterruptedException ex) {\n+\t\t\t\t// this should never happen so just swallow","path":"nailgun-server/src/main/java/com/martiansoftware/nailgun/NGSessionPool.java","position":137,"original_position":137,"commit_id":"707cbbba4800a42261276b32e38da32e46346e63","original_commit_id":"707cbbba4800a42261276b32e38da32e46346e63","user":{"login":"styurin","id":2167790,"node_id":"MDQ6VXNlcjIxNjc3OTA=","avatar_url":"https://avatars1.githubusercontent.com/u/2167790?v=4","gravatar_id":"","url":"https://api.github.com/users/styurin","html_url":"https://github.com/styurin","followers_url":"https://api.github.com/users/styurin/followers","following_url":"https://api.github.com/users/styurin/following{/other_user}","gists_url":"https://api.github.com/users/styurin/gists{/gist_id}","starred_url":"https://api.github.com/users/styurin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/styurin/subscriptions","organizations_url":"https://api.github.com/users/styurin/orgs","repos_url":"https://api.github.com/users/styurin/repos","events_url":"https://api.github.com/users/styurin/events{/privacy}","received_events_url":"https://api.github.com/users/styurin/received_events","type":"User","site_admin":false},"body":"That potentially can hide some problems. What if the server is interrupted? It will try to wait for all the sessions to finish.\r\n\r\nI suggest resetting the interrupted flag for the current thread and logging the exception. To me it seems that we need to stop waiting for the rest of the sessions on an interruption.","created_at":"2018-06-14T18:50:19Z","updated_at":"2018-06-14T18:54:06Z","html_url":"https://github.com/facebook/nailgun/pull/142#discussion_r195533520","pull_request_url":"https://api.github.com/repos/facebook/nailgun/pulls/142","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/facebook/nailgun/pulls/comments/195533520"},"html":{"href":"https://github.com/facebook/nailgun/pull/142#discussion_r195533520"},"pull_request":{"href":"https://api.github.com/repos/facebook/nailgun/pulls/142"}}},"pull_request":{"url":"https://api.github.com/repos/facebook/nailgun/pulls/142","id":194976275,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk0OTc2Mjc1","html_url":"https://github.com/facebook/nailgun/pull/142","diff_url":"https://github.com/facebook/nailgun/pull/142.diff","patch_url":"https://github.com/facebook/nailgun/pull/142.patch","issue_url":"https://api.github.com/repos/facebook/nailgun/issues/142","number":142,"state":"open","locked":false,"title":"Graceful termination for client and server","user":{"login":"sbalabanov","id":11744607,"node_id":"MDQ6VXNlcjExNzQ0NjA3","avatar_url":"https://avatars2.githubusercontent.com/u/11744607?v=4","gravatar_id":"","url":"https://api.github.com/users/sbalabanov","html_url":"https://github.com/sbalabanov","followers_url":"https://api.github.com/users/sbalabanov/followers","following_url":"https://api.github.com/users/sbalabanov/following{/other_user}","gists_url":"https://api.github.com/users/sbalabanov/gists{/gist_id}","starred_url":"https://api.github.com/users/sbalabanov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sbalabanov/subscriptions","organizations_url":"https://api.github.com/users/sbalabanov/orgs","repos_url":"https://api.github.com/users/sbalabanov/repos","events_url":"https://api.github.com/users/sbalabanov/events{/privacy}","received_events_url":"https://api.github.com/users/sbalabanov/received_events","type":"User","site_admin":false},"body":"In this change:\r\n- ng client gracefully process socket closure from server side to avoid race condition sending heartbeats to closed socket causing 'Broken Pipe' exception\r\n- ng client saves stack traces for original exceptions when it rethrows\r\n- ng-stop builtin command now properly returns before actual termination is happening\r\n- on termination, server wait for running nails to complete, signalling interruption to nail main thread\r\n- NgSession pool is finally fixed and waits for running jobs to complete before termination\r\n- NGServer is no longer printing to stderr which was the cause of contention (at least, for unit tests)","created_at":"2018-06-14T18:15:26Z","updated_at":"2018-06-14T18:54:06Z","closed_at":null,"merged_at":null,"merge_commit_sha":"6a66840ae85165dd614821f063b4ec5b6710b8d8","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/facebook/nailgun/pulls/142/commits","review_comments_url":"https://api.github.com/repos/facebook/nailgun/pulls/142/comments","review_comment_url":"https://api.github.com/repos/facebook/nailgun/pulls/comments{/number}","comments_url":"https://api.github.com/repos/facebook/nailgun/issues/142/comments","statuses_url":"https://api.github.com/repos/facebook/nailgun/statuses/707cbbba4800a42261276b32e38da32e46346e63","head":{"label":"sbalabanov:master","ref":"master","sha":"707cbbba4800a42261276b32e38da32e46346e63","user":{"login":"sbalabanov","id":11744607,"node_id":"MDQ6VXNlcjExNzQ0NjA3","avatar_url":"https://avatars2.githubusercontent.com/u/11744607?v=4","gravatar_id":"","url":"https://api.github.com/users/sbalabanov","html_url":"https://github.com/sbalabanov","followers_url":"https://api.github.com/users/sbalabanov/followers","following_url":"https://api.github.com/users/sbalabanov/following{/other_user}","gists_url":"https://api.github.com/users/sbalabanov/gists{/gist_id}","starred_url":"https://api.github.com/users/sbalabanov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sbalabanov/subscriptions","organizations_url":"https://api.github.com/users/sbalabanov/orgs","repos_url":"https://api.github.com/users/sbalabanov/repos","events_url":"https://api.github.com/users/sbalabanov/events{/privacy}","received_events_url":"https://api.github.com/users/sbalabanov/received_events","type":"User","site_admin":false},"repo":{"id":117041084,"node_id":"MDEwOlJlcG9zaXRvcnkxMTcwNDEwODQ=","name":"nailgun","full_name":"sbalabanov/nailgun","owner":{"login":"sbalabanov","id":11744607,"node_id":"MDQ6VXNlcjExNzQ0NjA3","avatar_url":"https://avatars2.githubusercontent.com/u/11744607?v=4","gravatar_id":"","url":"https://api.github.com/users/sbalabanov","html_url":"https://github.com/sbalabanov","followers_url":"https://api.github.com/users/sbalabanov/followers","following_url":"https://api.github.com/users/sbalabanov/following{/other_user}","gists_url":"https://api.github.com/users/sbalabanov/gists{/gist_id}","starred_url":"https://api.github.com/users/sbalabanov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sbalabanov/subscriptions","organizations_url":"https://api.github.com/users/sbalabanov/orgs","repos_url":"https://api.github.com/users/sbalabanov/repos","events_url":"https://api.github.com/users/sbalabanov/events{/privacy}","received_events_url":"https://api.github.com/users/sbalabanov/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/sbalabanov/nailgun","description":"Nailgun is a client, protocol, and server for running Java programs from the command line without incurring the JVM startup overhead.","fork":true,"url":"https://api.github.com/repos/sbalabanov/nailgun","forks_url":"https://api.github.com/repos/sbalabanov/nailgun/forks","keys_url":"https://api.github.com/repos/sbalabanov/nailgun/keys{/key_id}","collaborators_url":"https://api.github.com/repos/sbalabanov/nailgun/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/sbalabanov/nailgun/teams","hooks_url":"https://api.github.com/repos/sbalabanov/nailgun/hooks","issue_events_url":"https://api.github.com/repos/sbalabanov/nailgun/issues/events{/number}","events_url":"https://api.github.com/repos/sbalabanov/nailgun/events","assignees_url":"https://api.github.com/repos/sbalabanov/nailgun/assignees{/user}","branches_url":"https://api.github.com/repos/sbalabanov/nailgun/branches{/branch}","tags_url":"https://api.github.com/repos/sbalabanov/nailgun/tags","blobs_url":"https://api.github.com/repos/sbalabanov/nailgun/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/sbalabanov/nailgun/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/sbalabanov/nailgun/git/refs{/sha}","trees_url":"https://api.github.com/repos/sbalabanov/nailgun/git/trees{/sha}","statuses_url":"https://api.github.com/repos/sbalabanov/nailgun/statuses/{sha}","languages_url":"https://api.github.com/repos/sbalabanov/nailgun/languages","stargazers_url":"https://api.github.com/repos/sbalabanov/nailgun/stargazers","contributors_url":"https://api.github.com/repos/sbalabanov/nailgun/contributors","subscribers_url":"https://api.github.com/repos/sbalabanov/nailgun/subscribers","subscription_url":"https://api.github.com/repos/sbalabanov/nailgun/subscription","commits_url":"https://api.github.com/repos/sbalabanov/nailgun/commits{/sha}","git_commits_url":"https://api.github.com/repos/sbalabanov/nailgun/git/commits{/sha}","comments_url":"https://api.github.com/repos/sbalabanov/nailgun/comments{/number}","issue_comment_url":"https://api.github.com/repos/sbalabanov/nailgun/issues/comments{/number}","contents_url":"https://api.github.com/repos/sbalabanov/nailgun/contents/{+path}","compare_url":"https://api.github.com/repos/sbalabanov/nailgun/compare/{base}...{head}","merges_url":"https://api.github.com/repos/sbalabanov/nailgun/merges","archive_url":"https://api.github.com/repos/sbalabanov/nailgun/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/sbalabanov/nailgun/downloads","issues_url":"https://api.github.com/repos/sbalabanov/nailgun/issues{/number}","pulls_url":"https://api.github.com/repos/sbalabanov/nailgun/pulls{/number}","milestones_url":"https://api.github.com/repos/sbalabanov/nailgun/milestones{/number}","notifications_url":"https://api.github.com/repos/sbalabanov/nailgun/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/sbalabanov/nailgun/labels{/name}","releases_url":"https://api.github.com/repos/sbalabanov/nailgun/releases{/id}","deployments_url":"https://api.github.com/repos/sbalabanov/nailgun/deployments","created_at":"2018-01-11T02:47:29Z","updated_at":"2018-06-14T18:08:43Z","pushed_at":"2018-06-14T18:08:42Z","git_url":"git://github.com/sbalabanov/nailgun.git","ssh_url":"git@github.com:sbalabanov/nailgun.git","clone_url":"https://github.com/sbalabanov/nailgun.git","svn_url":"https://github.com/sbalabanov/nailgun","homepage":null,"size":254,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":{"key":"apache-2.0","name":"Apache License 2.0","spdx_id":"Apache-2.0","url":"https://api.github.com/licenses/apache-2.0","node_id":"MDc6TGljZW5zZTI="},"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"facebook:master","ref":"master","sha":"604b49b69c48f782cefded77deb2828b68bc6a75","user":{"login":"facebook","id":69631,"node_id":"MDEyOk9yZ2FuaXphdGlvbjY5NjMx","avatar_url":"https://avatars3.githubusercontent.com/u/69631?v=4","gravatar_id":"","url":"https://api.github.com/users/facebook","html_url":"https://github.com/facebook","followers_url":"https://api.github.com/users/facebook/followers","following_url":"https://api.github.com/users/facebook/following{/other_user}","gists_url":"https://api.github.com/users/facebook/gists{/gist_id}","starred_url":"https://api.github.com/users/facebook/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/facebook/subscriptions","organizations_url":"https://api.github.com/users/facebook/orgs","repos_url":"https://api.github.com/users/facebook/repos","events_url":"https://api.github.com/users/facebook/events{/privacy}","received_events_url":"https://api.github.com/users/facebook/received_events","type":"Organization","site_admin":false},"repo":{"id":6833345,"node_id":"MDEwOlJlcG9zaXRvcnk2ODMzMzQ1","name":"nailgun","full_name":"facebook/nailgun","owner":{"login":"facebook","id":69631,"node_id":"MDEyOk9yZ2FuaXphdGlvbjY5NjMx","avatar_url":"https://avatars3.githubusercontent.com/u/69631?v=4","gravatar_id":"","url":"https://api.github.com/users/facebook","html_url":"https://github.com/facebook","followers_url":"https://api.github.com/users/facebook/followers","following_url":"https://api.github.com/users/facebook/following{/other_user}","gists_url":"https://api.github.com/users/facebook/gists{/gist_id}","starred_url":"https://api.github.com/users/facebook/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/facebook/subscriptions","organizations_url":"https://api.github.com/users/facebook/orgs","repos_url":"https://api.github.com/users/facebook/repos","events_url":"https://api.github.com/users/facebook/events{/privacy}","received_events_url":"https://api.github.com/users/facebook/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/facebook/nailgun","description":"Nailgun is a client, protocol, and server for running Java programs from the command line without incurring the JVM startup overhead.","fork":false,"url":"https://api.github.com/repos/facebook/nailgun","forks_url":"https://api.github.com/repos/facebook/nailgun/forks","keys_url":"https://api.github.com/repos/facebook/nailgun/keys{/key_id}","collaborators_url":"https://api.github.com/repos/facebook/nailgun/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/facebook/nailgun/teams","hooks_url":"https://api.github.com/repos/facebook/nailgun/hooks","issue_events_url":"https://api.github.com/repos/facebook/nailgun/issues/events{/number}","events_url":"https://api.github.com/repos/facebook/nailgun/events","assignees_url":"https://api.github.com/repos/facebook/nailgun/assignees{/user}","branches_url":"https://api.github.com/repos/facebook/nailgun/branches{/branch}","tags_url":"https://api.github.com/repos/facebook/nailgun/tags","blobs_url":"https://api.github.com/repos/facebook/nailgun/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/facebook/nailgun/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/facebook/nailgun/git/refs{/sha}","trees_url":"https://api.github.com/repos/facebook/nailgun/git/trees{/sha}","statuses_url":"https://api.github.com/repos/facebook/nailgun/statuses/{sha}","languages_url":"https://api.github.com/repos/facebook/nailgun/languages","stargazers_url":"https://api.github.com/repos/facebook/nailgun/stargazers","contributors_url":"https://api.github.com/repos/facebook/nailgun/contributors","subscribers_url":"https://api.github.com/repos/facebook/nailgun/subscribers","subscription_url":"https://api.github.com/repos/facebook/nailgun/subscription","commits_url":"https://api.github.com/repos/facebook/nailgun/commits{/sha}","git_commits_url":"https://api.github.com/repos/facebook/nailgun/git/commits{/sha}","comments_url":"https://api.github.com/repos/facebook/nailgun/comments{/number}","issue_comment_url":"https://api.github.com/repos/facebook/nailgun/issues/comments{/number}","contents_url":"https://api.github.com/repos/facebook/nailgun/contents/{+path}","compare_url":"https://api.github.com/repos/facebook/nailgun/compare/{base}...{head}","merges_url":"https://api.github.com/repos/facebook/nailgun/merges","archive_url":"https://api.github.com/repos/facebook/nailgun/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/facebook/nailgun/downloads","issues_url":"https://api.github.com/repos/facebook/nailgun/issues{/number}","pulls_url":"https://api.github.com/repos/facebook/nailgun/pulls{/number}","milestones_url":"https://api.github.com/repos/facebook/nailgun/milestones{/number}","notifications_url":"https://api.github.com/repos/facebook/nailgun/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/facebook/nailgun/labels{/name}","releases_url":"https://api.github.com/repos/facebook/nailgun/releases{/id}","deployments_url":"https://api.github.com/repos/facebook/nailgun/deployments","created_at":"2012-11-23T21:28:43Z","updated_at":"2018-06-13T04:59:50Z","pushed_at":"2018-06-14T18:15:26Z","git_url":"git://github.com/facebook/nailgun.git","ssh_url":"git@github.com:facebook/nailgun.git","clone_url":"https://github.com/facebook/nailgun.git","svn_url":"https://github.com/facebook/nailgun","homepage":null,"size":250,"stargazers_count":462,"watchers_count":462,"language":"Java","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":80,"mirror_url":null,"archived":false,"open_issues_count":7,"license":{"key":"apache-2.0","name":"Apache License 2.0","spdx_id":"Apache-2.0","url":"https://api.github.com/licenses/apache-2.0","node_id":"MDc6TGljZW5zZTI="},"forks":80,"open_issues":7,"watchers":462,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/facebook/nailgun/pulls/142"},"html":{"href":"https://github.com/facebook/nailgun/pull/142"},"issue":{"href":"https://api.github.com/repos/facebook/nailgun/issues/142"},"comments":{"href":"https://api.github.com/repos/facebook/nailgun/issues/142/comments"},"review_comments":{"href":"https://api.github.com/repos/facebook/nailgun/pulls/142/comments"},"review_comment":{"href":"https://api.github.com/repos/facebook/nailgun/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/facebook/nailgun/pulls/142/commits"},"statuses":{"href":"https://api.github.com/repos/facebook/nailgun/statuses/707cbbba4800a42261276b32e38da32e46346e63"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"styurin"}}