{"action":"created","comment":{"url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/comments/31520518","id":31520518,"diff_hunk":"@@ -952,28 +995,46 @@ namespace zmq {\n     if(zmq_socket_monitor(socket->socket_, addr, ZMQ_EVENT_ALL) != -1) {\n       socket->monitor_socket_ = zmq_socket (context->context_, ZMQ_PAIR);\n       zmq_connect (socket->monitor_socket_, addr);\n-\n+      socket->timer_interval_ = timer_interval;\n+      socket->num_of_events_ = num_of_events;\n       socket->monitor_handle_ = new uv_timer_t;\n-\n       socket->monitor_handle_->data = socket;\n \n       uv_timer_init(uv_default_loop(), socket->monitor_handle_);\n-      uv_timer_start(socket->monitor_handle_, reinterpret_cast<uv_timer_cb>(Socket::UV_MonitorCallback), timer_interval, timer_interval);\n+      uv_timer_start(socket->monitor_handle_, reinterpret_cast<uv_timer_cb>(Socket::UV_MonitorCallback), timer_interval, 0);\n     }\n \n     NanReturnUndefined();\n   }\n \n+  void\n+  Socket::Unmonitor() {\n+    // Make sure we are monitoring\n+    if (this->monitor_socket_ == NULL) {\n+      return;\n+    }\n+\n+    // Passing NULL as addr will tell zmq to stop monitor\n+    zmq_socket_monitor(this->socket_, NULL, ZMQ_EVENT_ALL);\n+\n+    // Close the monitor socket and stop timer\n+    if (zmq_close(this->monitor_socket_) < 0)\n+      throw std::runtime_error(ErrorMessage());","path":"binding.cc","position":222,"original_position":222,"commit_id":"2f7eda3b4c58813fd61d8c601b8cf527d1a07608","original_commit_id":"2f7eda3b4c58813fd61d8c601b8cf527d1a07608","user":{"login":"ValYouW","id":4539636,"avatar_url":"https://avatars.githubusercontent.com/u/4539636?v=3","gravatar_id":"","url":"https://api.github.com/users/ValYouW","html_url":"https://github.com/ValYouW","followers_url":"https://api.github.com/users/ValYouW/followers","following_url":"https://api.github.com/users/ValYouW/following{/other_user}","gists_url":"https://api.github.com/users/ValYouW/gists{/gist_id}","starred_url":"https://api.github.com/users/ValYouW/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ValYouW/subscriptions","organizations_url":"https://api.github.com/users/ValYouW/orgs","repos_url":"https://api.github.com/users/ValYouW/repos","events_url":"https://api.github.com/users/ValYouW/events{/privacy}","received_events_url":"https://api.github.com/users/ValYouW/received_events","type":"User","site_admin":false},"body":"I didn't touch that, whether or not we want this module to throw errors is a good question, I think that we should just raise an error event and let the use close the socket and create a new one. Throwing exception could crash the user process, which is a little too much...\r\n\r\nMy concern is L1018, calling `zmq_socket_monitor` with NULL address in order to stop monitor, I didn't find anything in the docs, only in the zmq code: https://github.com/zeromq/libzmq/blob/master/src/socket_base.cpp#L1380\r\n","created_at":"2015-06-02T13:16:45Z","updated_at":"2015-06-02T13:16:45Z","html_url":"https://github.com/JustinTulloss/zeromq.node/pull/424#discussion_r31520518","pull_request_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424","_links":{"self":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/comments/31520518"},"html":{"href":"https://github.com/JustinTulloss/zeromq.node/pull/424#discussion_r31520518"},"pull_request":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424"}}},"pull_request":{"url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424","id":35675610,"html_url":"https://github.com/JustinTulloss/zeromq.node/pull/424","diff_url":"https://github.com/JustinTulloss/zeromq.node/pull/424.diff","patch_url":"https://github.com/JustinTulloss/zeromq.node/pull/424.patch","issue_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues/424","number":424,"state":"open","locked":false,"title":"Read all available monitor messages on each timer callback","user":{"login":"ValYouW","id":4539636,"avatar_url":"https://avatars.githubusercontent.com/u/4539636?v=3","gravatar_id":"","url":"https://api.github.com/users/ValYouW","html_url":"https://github.com/ValYouW","followers_url":"https://api.github.com/users/ValYouW/followers","following_url":"https://api.github.com/users/ValYouW/following{/other_user}","gists_url":"https://api.github.com/users/ValYouW/gists{/gist_id}","starred_url":"https://api.github.com/users/ValYouW/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ValYouW/subscriptions","organizations_url":"https://api.github.com/users/ValYouW/orgs","repos_url":"https://api.github.com/users/ValYouW/repos","events_url":"https://api.github.com/users/ValYouW/events{/privacy}","received_events_url":"https://api.github.com/users/ValYouW/received_events","type":"User","site_admin":false},"body":"Currently, on each callback of the monitor timer, only one message is read from the monitor socket, this can lead to a situation where the events reporting start to fall behind.\r\nFor example (our case):\r\n* A dealer socker\r\n* The monitor interval is set to 1000ms\r\n* The ZMQ_RECONNECT_IVL is set to 1000ms, so is the ZMQ_RECONNECT_IVL_MAX.\r\n* The server goes down, zmq tries to reconnect, about every 1 second it sends 3 events on the monitor socket (connect_retry, connect_delay, close), but only 1 message is being read (every 1 second), eventually the server goes up, the socket gets reconnected (can see the port is taken), but the monitor events are falling behind and it can take quite some time until the `connected` event will be reached.\r\n\r\nThis PR makes the following changes:\r\n1. On each monitor timer callback it tries to read all the available messages from the monitor socket and raise the events\r\n2. Since each monitor callback now can take more time (as we read all messages and not just one), I removed the interval from `uv_timer_start` and manually reset the timer when all messages from the monitor socket processed (basically switching from `setInterval` to `setTimeout`)\r\n3. See the comment on line 461, are we missing a call to `zmq_msg_close(&msg2)` there ??\r\n\r\nWhat do you think?","created_at":"2015-05-18T21:30:25Z","updated_at":"2015-06-02T13:16:45Z","closed_at":null,"merged_at":null,"merge_commit_sha":"4a376f5183f7478ce772fe74ca4edb1fa0447da4","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424/commits","review_comments_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424/comments","review_comment_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/comments{/number}","comments_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues/424/comments","statuses_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/statuses/2f7eda3b4c58813fd61d8c601b8cf527d1a07608","head":{"label":"ValYouW:master","ref":"master","sha":"2f7eda3b4c58813fd61d8c601b8cf527d1a07608","user":{"login":"ValYouW","id":4539636,"avatar_url":"https://avatars.githubusercontent.com/u/4539636?v=3","gravatar_id":"","url":"https://api.github.com/users/ValYouW","html_url":"https://github.com/ValYouW","followers_url":"https://api.github.com/users/ValYouW/followers","following_url":"https://api.github.com/users/ValYouW/following{/other_user}","gists_url":"https://api.github.com/users/ValYouW/gists{/gist_id}","starred_url":"https://api.github.com/users/ValYouW/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ValYouW/subscriptions","organizations_url":"https://api.github.com/users/ValYouW/orgs","repos_url":"https://api.github.com/users/ValYouW/repos","events_url":"https://api.github.com/users/ValYouW/events{/privacy}","received_events_url":"https://api.github.com/users/ValYouW/received_events","type":"User","site_admin":false},"repo":{"id":31646743,"name":"zeromq.node","full_name":"ValYouW/zeromq.node","owner":{"login":"ValYouW","id":4539636,"avatar_url":"https://avatars.githubusercontent.com/u/4539636?v=3","gravatar_id":"","url":"https://api.github.com/users/ValYouW","html_url":"https://github.com/ValYouW","followers_url":"https://api.github.com/users/ValYouW/followers","following_url":"https://api.github.com/users/ValYouW/following{/other_user}","gists_url":"https://api.github.com/users/ValYouW/gists{/gist_id}","starred_url":"https://api.github.com/users/ValYouW/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ValYouW/subscriptions","organizations_url":"https://api.github.com/users/ValYouW/orgs","repos_url":"https://api.github.com/users/ValYouW/repos","events_url":"https://api.github.com/users/ValYouW/events{/privacy}","received_events_url":"https://api.github.com/users/ValYouW/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/ValYouW/zeromq.node","description":"Node.js bindings to the zeromq library","fork":true,"url":"https://api.github.com/repos/ValYouW/zeromq.node","forks_url":"https://api.github.com/repos/ValYouW/zeromq.node/forks","keys_url":"https://api.github.com/repos/ValYouW/zeromq.node/keys{/key_id}","collaborators_url":"https://api.github.com/repos/ValYouW/zeromq.node/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/ValYouW/zeromq.node/teams","hooks_url":"https://api.github.com/repos/ValYouW/zeromq.node/hooks","issue_events_url":"https://api.github.com/repos/ValYouW/zeromq.node/issues/events{/number}","events_url":"https://api.github.com/repos/ValYouW/zeromq.node/events","assignees_url":"https://api.github.com/repos/ValYouW/zeromq.node/assignees{/user}","branches_url":"https://api.github.com/repos/ValYouW/zeromq.node/branches{/branch}","tags_url":"https://api.github.com/repos/ValYouW/zeromq.node/tags","blobs_url":"https://api.github.com/repos/ValYouW/zeromq.node/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/ValYouW/zeromq.node/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/ValYouW/zeromq.node/git/refs{/sha}","trees_url":"https://api.github.com/repos/ValYouW/zeromq.node/git/trees{/sha}","statuses_url":"https://api.github.com/repos/ValYouW/zeromq.node/statuses/{sha}","languages_url":"https://api.github.com/repos/ValYouW/zeromq.node/languages","stargazers_url":"https://api.github.com/repos/ValYouW/zeromq.node/stargazers","contributors_url":"https://api.github.com/repos/ValYouW/zeromq.node/contributors","subscribers_url":"https://api.github.com/repos/ValYouW/zeromq.node/subscribers","subscription_url":"https://api.github.com/repos/ValYouW/zeromq.node/subscription","commits_url":"https://api.github.com/repos/ValYouW/zeromq.node/commits{/sha}","git_commits_url":"https://api.github.com/repos/ValYouW/zeromq.node/git/commits{/sha}","comments_url":"https://api.github.com/repos/ValYouW/zeromq.node/comments{/number}","issue_comment_url":"https://api.github.com/repos/ValYouW/zeromq.node/issues/comments{/number}","contents_url":"https://api.github.com/repos/ValYouW/zeromq.node/contents/{+path}","compare_url":"https://api.github.com/repos/ValYouW/zeromq.node/compare/{base}...{head}","merges_url":"https://api.github.com/repos/ValYouW/zeromq.node/merges","archive_url":"https://api.github.com/repos/ValYouW/zeromq.node/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/ValYouW/zeromq.node/downloads","issues_url":"https://api.github.com/repos/ValYouW/zeromq.node/issues{/number}","pulls_url":"https://api.github.com/repos/ValYouW/zeromq.node/pulls{/number}","milestones_url":"https://api.github.com/repos/ValYouW/zeromq.node/milestones{/number}","notifications_url":"https://api.github.com/repos/ValYouW/zeromq.node/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/ValYouW/zeromq.node/labels{/name}","releases_url":"https://api.github.com/repos/ValYouW/zeromq.node/releases{/id}","created_at":"2015-03-04T08:40:24Z","updated_at":"2015-03-07T13:36:43Z","pushed_at":"2015-06-02T08:29:12Z","git_url":"git://github.com/ValYouW/zeromq.node.git","ssh_url":"git@github.com:ValYouW/zeromq.node.git","clone_url":"https://github.com/ValYouW/zeromq.node.git","svn_url":"https://github.com/ValYouW/zeromq.node","homepage":"","size":1820,"stargazers_count":0,"watchers_count":0,"language":"JavaScript","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"JustinTulloss:master","ref":"master","sha":"34450372ea338b73ca99533760ff04e066280e22","user":{"login":"JustinTulloss","id":23734,"avatar_url":"https://avatars.githubusercontent.com/u/23734?v=3","gravatar_id":"","url":"https://api.github.com/users/JustinTulloss","html_url":"https://github.com/JustinTulloss","followers_url":"https://api.github.com/users/JustinTulloss/followers","following_url":"https://api.github.com/users/JustinTulloss/following{/other_user}","gists_url":"https://api.github.com/users/JustinTulloss/gists{/gist_id}","starred_url":"https://api.github.com/users/JustinTulloss/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JustinTulloss/subscriptions","organizations_url":"https://api.github.com/users/JustinTulloss/orgs","repos_url":"https://api.github.com/users/JustinTulloss/repos","events_url":"https://api.github.com/users/JustinTulloss/events{/privacy}","received_events_url":"https://api.github.com/users/JustinTulloss/received_events","type":"User","site_admin":false},"repo":{"id":595090,"name":"zeromq.node","full_name":"JustinTulloss/zeromq.node","owner":{"login":"JustinTulloss","id":23734,"avatar_url":"https://avatars.githubusercontent.com/u/23734?v=3","gravatar_id":"","url":"https://api.github.com/users/JustinTulloss","html_url":"https://github.com/JustinTulloss","followers_url":"https://api.github.com/users/JustinTulloss/followers","following_url":"https://api.github.com/users/JustinTulloss/following{/other_user}","gists_url":"https://api.github.com/users/JustinTulloss/gists{/gist_id}","starred_url":"https://api.github.com/users/JustinTulloss/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JustinTulloss/subscriptions","organizations_url":"https://api.github.com/users/JustinTulloss/orgs","repos_url":"https://api.github.com/users/JustinTulloss/repos","events_url":"https://api.github.com/users/JustinTulloss/events{/privacy}","received_events_url":"https://api.github.com/users/JustinTulloss/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/JustinTulloss/zeromq.node","description":"Node.js bindings to the zeromq library","fork":false,"url":"https://api.github.com/repos/JustinTulloss/zeromq.node","forks_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/forks","keys_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/keys{/key_id}","collaborators_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/teams","hooks_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/hooks","issue_events_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues/events{/number}","events_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/events","assignees_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/assignees{/user}","branches_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/branches{/branch}","tags_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/tags","blobs_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/git/refs{/sha}","trees_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/git/trees{/sha}","statuses_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/statuses/{sha}","languages_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/languages","stargazers_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/stargazers","contributors_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/contributors","subscribers_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/subscribers","subscription_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/subscription","commits_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/commits{/sha}","git_commits_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/git/commits{/sha}","comments_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/comments{/number}","issue_comment_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues/comments{/number}","contents_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/contents/{+path}","compare_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/compare/{base}...{head}","merges_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/merges","archive_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/downloads","issues_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues{/number}","pulls_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls{/number}","milestones_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/milestones{/number}","notifications_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/labels{/name}","releases_url":"https://api.github.com/repos/JustinTulloss/zeromq.node/releases{/id}","created_at":"2010-04-05T07:23:15Z","updated_at":"2015-06-02T08:50:28Z","pushed_at":"2015-06-02T08:29:13Z","git_url":"git://github.com/JustinTulloss/zeromq.node.git","ssh_url":"git@github.com:JustinTulloss/zeromq.node.git","clone_url":"https://github.com/JustinTulloss/zeromq.node.git","svn_url":"https://github.com/JustinTulloss/zeromq.node","homepage":"","size":4673,"stargazers_count":1116,"watchers_count":1116,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":219,"mirror_url":null,"open_issues_count":117,"forks":219,"open_issues":117,"watchers":1116,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424"},"html":{"href":"https://github.com/JustinTulloss/zeromq.node/pull/424"},"issue":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues/424"},"comments":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/issues/424/comments"},"review_comments":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424/comments"},"review_comment":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/pulls/424/commits"},"statuses":{"href":"https://api.github.com/repos/JustinTulloss/zeromq.node/statuses/2f7eda3b4c58813fd61d8c601b8cf527d1a07608"}}}}