Repository navigation
Feat/enquire link keepalive - #12
Merged
Merged
Conversation
An incoming enquire_link was only emitted as an event, leaving every user to answer it by hand, and auto_enquire_link_period sent enquire_link without ever looking at the enquire_link_resp, so a dead link stayed open until TCP gave up on it, which can take many minutes. auto_enquire_link_period passed to createServer() was silently ignored. New options, accepted by both connect() and createServer(): - auto_enquire_link_response answers incoming enquire_link, before the 'pdu' and 'enquire_link' events are emitted. - enquire_link_timeout arms a timer for every keepalive enquire_link. When it fires, the session emits enquire_link_timeout with the unanswered PDU and is destroyed, unless close_on_enquire_link_timeout is false. - auto_enquire_link_period now also works on server sessions. The keepalive enquire_link only wait for enquire_link_timeout: response_timeout is for the requests the user sends, so they are tracked without its timer and never show up in 'response_timeout' or 'response_aborted' events. Otherwise a response_timeout shorter than enquire_link_timeout would settle them first and switch the link check off. startEnquireLink(period, [timeout]) / stopEnquireLink() and the autoEnquireLinkResponse property control the same at runtime. Everything defaults to the previous behaviour: answering automatically by default would double the response for everyone who already answers in their own handler. Lifecycle details: - A keepalive registers a response callback only when there is a timeout to clear, and the callback is removed on timeout, on stop and when the write fails, so nothing accumulates against a silent peer. - Any response with the right sequence number counts as an answer, including generic_nack, which some SMSCs send for an enquire_link before the bind. - close() stops sending but keeps awaiting an enquire_link already in flight: a peer that stopped answering doesn't send its FIN either, and that timeout is what drops the half-open session. destroy() stops the keepalive synchronously. Replacing the schedule keeps the enquire_link already sent awaited. - The schedule only starts on a writable socket, so it doesn't start when close() was called from a 'connect' listener, and a negative or non-numeric timeout or period is ignored instead of firing at once.
connect() reconnected by calling connect() again on the previous socket. A net.Socket that was closed can connect again, but never emits 'readable' again, so a reconnected session sent PDUs and never read a single one: responses and incoming requests were silently lost. The connection was also only half reset: a partly received PDU kept its command length and corrupted the first PDU of the new connection, the bytesRead baseline stayed at the old socket's count, closed stayed true, and the connect timeout was not re-armed. connect() now opens a new socket (net or tls, as in the constructor) and resets all per-connection state. Socket listeners only act while their socket is the session's socket: reconnecting from an 'error' listener used to be followed by the failed connection's 'close', which made the session look closed again. They stay attached so that a late error of a replaced socket is swallowed instead of crashing the process. A previous connection that is still open or still connecting is destroyed instead of failing with EISCONN / EALREADY, and a server session throws a clear error instead of Node's ERR_MISSING_ARGS. The enquire_link schedule and all listeners carry over to the new connection. The new socket is created before anything else is touched, so options that net.connect() or tls.connect() reject leave the session as it was. Requests waiting for a response fail with ESESSIONCLOSED when response_timeout is set, as on close, and the socket to replace is only picked after their failure callbacks ran, since those may reconnect on their own. A connection replaced from such a callback, which runs while its 'close' is handled, no longer emits that 'close'; and a connection opened from an enquire_link_timeout listener is no longer destroyed by close_on_enquire_link_timeout right after. A write that fails on the replaced socket can't drop the callback of a new request reusing its sequence number, since pending entries are only removed by identity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
enquire_linkwas only emitted as an event; every user had to answer it by hand.auto_enquire_link_periodsentenquire_linkbut never checked the response, so a dead link stayed "open" until TCP gave up on it, which can take many minutes. Passed tocreateServer(), the option was silently ignored.session.connect()could not reconnect: it reused the closed socket, which never emits'readable'again, so a reconnected session sent PDUs and never received one.Changes
enquire_link (minor)
New options, accepted by both
smpp.connect()andsmpp.createServer():auto_enquire_link_responsefalseenquire_linkbefore the'pdu'/'enquire_link'eventsauto_enquire_link_periodenquire_link_timeoutenquire_link_timeoutwith the unanswered PDUclose_on_enquire_link_timeouttrueenquire_link_timeoutsession.startEnquireLink(period, [timeout]),session.stopEnquireLink()andsession.autoEnquireLinkResponse.enquire_linkare not subject toresponse_timeout. They only wait forenquire_link_timeout, and never show up in'response_timeout'or'response_aborted'.close()stops sending, but keeps awaiting anenquire_linkalready in flight, so a half-open peer is still dropped.destroy()stops the keepalive synchronously.session.connect() (patch)
closed.connectTimeoutis re-armed for every attempt.'close'after reconnecting from'error'.ESESSIONCLOSEDwhenresponse_timeoutis set.enquire_link_timeoutlistener works: the new connection is not destroyed.net.connect()/tls.connect()leave the session as it was.Testing
'connection'deferred, to simulate macOS ordering.