Skip to content

Commit 7eb5d43

Browse files
bigmontzbigmontz
authored
Fix receiveTimeout behaviour (#903)
The previous implementation of the `timeout` was not considering if requests were being treated by the server or not. This behaviour causes connections being wrongly close by inactivity. For fixing this issue, the `timeout` configuration changed to only be applied whenever there are ongoing requests.
1 parent 919469a commit 7eb5d43

File tree

7 files changed

+228
-4
lines changed

7 files changed

+228
-4
lines changed

‎packages/bolt-connection/src/bolt/response-handler.js

+3
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ export default class ResponseHandler {
7878
this._transformMetadata = transformMetadata || NO_OP_IDENTITY
7979
this._observer = Object.assign(
8080
{
81+
onPendingObserversChange: NO_OP,
8182
onError: NO_OP,
8283
onFailure: NO_OP,
8384
onErrorApplyTransformation: NO_OP_IDENTITY
@@ -156,6 +157,7 @@ export default class ResponseHandler {
156157
*/
157158
_updateCurrentObserver () {
158159
this._currentObserver = this._pendingObservers.shift()
160+
this._observer.onPendingObserversChange(this._pendingObservers.length)
159161
}
160162

161163
_queueObserver (observer) {
@@ -168,6 +170,7 @@ export default class ResponseHandler {
168170
} else {
169171
this._pendingObservers.push(observer)
170172
}
173+
this._observer.onPendingObserversChange(this._pendingObservers.length)
171174
return true
172175
}
173176

‎packages/bolt-connection/src/channel/browser/browser-channel.js

+12
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,18 @@ export default class WebSocketChannel {
181181
*/
182182
setupReceiveTimeout (receiveTimeout) {}
183183

184+
/**
185+
* Stops the receive timeout for the channel.
186+
*/
187+
stopReceiveTimeout() {
188+
}
189+
190+
/**
191+
* Start the receive timeout for the channel.
192+
*/
193+
startReceiveTimeout () {
194+
}
195+
184196
/**
185197
* Set connection timeout on the given WebSocket, if configured.
186198
* @return {number} the timeout id or null.

‎packages/bolt-connection/src/channel/node/node-channel.js

+23-1
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,8 @@ export default class NodeChannel {
242242
this
243243
)
244244
this._connectionErrorCode = config.connectionErrorCode
245+
this._receiveTimeout = null
246+
this._receiveTimeoutStarted = false
245247

246248
this._conn = connect(
247249
config,
@@ -353,7 +355,27 @@ export default class NodeChannel {
353355
)
354356
})
355357

356-
this._conn.setTimeout(receiveTimeout)
358+
this._receiveTimeout = receiveTimeout
359+
}
360+
361+
/**
362+
* Stops the receive timeout for the channel.
363+
*/
364+
stopReceiveTimeout() {
365+
if (this._receiveTimeout !== null && this._receiveTimeoutStarted) {
366+
this._receiveTimeoutStarted = false
367+
this._conn.setTimeout(0)
368+
}
369+
}
370+
371+
/**
372+
* Start the receive timeout for the channel.
373+
*/
374+
startReceiveTimeout () {
375+
if (this._receiveTimeout !== null && !this._receiveTimeoutStarted) {
376+
this._receiveTimeoutStarted = true
377+
this._conn.setTimeout(this._receiveTimeout)
378+
}
357379
}
358380

359381
/**

‎packages/bolt-connection/src/connection/connection-channel.js

+13
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ export function createChannelConnection (
6969
server: conn.server,
7070
log: conn.logger,
7171
observer: {
72+
onPendingObserversChange: conn._handleOngoingRequestsNumberChange.bind(conn),
7273
onError: conn._handleFatalError.bind(conn),
7374
onFailure: conn._resetOnFailure.bind(conn),
7475
onProtocolError: conn._handleProtocolError.bind(conn),
@@ -350,6 +351,18 @@ export default class ChannelConnection extends Connection {
350351
return !this._isBroken && this._ch._open
351352
}
352353

354+
/**
355+
* Starts and stops the receive timeout timer.
356+
* @param {number} requestsNumber Ongoing requests number
357+
*/
358+
_handleOngoingRequestsNumberChange(requestsNumber) {
359+
if (requestsNumber === 0) {
360+
this._ch.stopReceiveTimeout()
361+
} else {
362+
this._ch.startReceiveTimeout()
363+
}
364+
}
365+
353366
/**
354367
* Call close on the channel.
355368
* @returns {Promise<void>} - A promise that will be resolved when the underlying channel is closed.

‎packages/bolt-connection/test/channel/node/node-channel.test.js

+118-2
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,13 @@ describe('NodeChannel', () => {
5959
})
6060

6161
describe('.setupReceiveTimeout()', () => {
62-
it('should call socket.setTimeout(receiveTimeout)', () => {
62+
it('should not call socket.setTimeout(receiveTimeout)', () => {
6363
const receiveTimeout = 42
6464
const channel = createMockedChannel(true)
6565

6666
channel.setupReceiveTimeout(receiveTimeout)
6767

68-
expect(channel._conn.getCalls().setTimeout[1]).toEqual([receiveTimeout])
68+
expect(channel._conn.getCalls().setTimeout.length).toEqual(1)
6969
})
7070

7171
it('should unsubscribe to the on connect and on timeout created on the create socket', () => {
@@ -108,6 +108,122 @@ describe('NodeChannel', () => {
108108
expect(channel._conn.getCalls().off).toEqual([])
109109
})
110110
})
111+
112+
describe('.startReceiveTimeout()', () => {
113+
describe('receive timeout is setup', () => {
114+
it('should call socket.setTimeout(receiveTimeout) when called first', () => {
115+
const { receiveTimeout, channel } = setup()
116+
117+
channel.startReceiveTimeout()
118+
119+
expect(channel._conn.getCalls().setTimeout.length).toEqual(2)
120+
expect(channel._conn.getCalls().setTimeout[1]).toEqual([receiveTimeout])
121+
})
122+
123+
it ('should not call socket.setTimeout(receiveTimeout) if stream already started', () => {
124+
const { receiveTimeout, channel } = setup()
125+
126+
// setup
127+
channel.startReceiveTimeout()
128+
expect(channel._conn.getCalls().setTimeout.length).toEqual(2)
129+
expect(channel._conn.getCalls().setTimeout[1]).toEqual([receiveTimeout])
130+
131+
// start again
132+
channel.startReceiveTimeout()
133+
134+
expect(channel._conn.getCalls().setTimeout.length).toEqual(2)
135+
expect(channel._conn.getCalls().setTimeout[1]).toEqual([receiveTimeout])
136+
})
137+
138+
it ('should call socket.setTimeout(receiveTimeout) when after stop', () => {
139+
const { receiveTimeout, channel } = setup()
140+
141+
// setup
142+
channel.startReceiveTimeout()
143+
expect(channel._conn.getCalls().setTimeout.length).toEqual(2)
144+
expect(channel._conn.getCalls().setTimeout[1]).toEqual([receiveTimeout])
145+
channel.stopReceiveTimeout()
146+
expect(channel._conn.getCalls().setTimeout.length).toEqual(3)
147+
expect(channel._conn.getCalls().setTimeout[2]).toEqual([0])
148+
149+
// start again
150+
channel.startReceiveTimeout()
151+
152+
expect(channel._conn.getCalls().setTimeout.length).toEqual(4)
153+
expect(channel._conn.getCalls().setTimeout[3]).toEqual([receiveTimeout])
154+
})
155+
156+
function setup () {
157+
const channel = createMockedChannel(true)
158+
const receiveTimeout = 42
159+
channel.setupReceiveTimeout(receiveTimeout)
160+
return {channel, receiveTimeout}
161+
}
162+
})
163+
164+
describe('receive timemout is not setup', () => {
165+
it ('should call not socket.setTimeout(receiveTimeout) when not started', () => {
166+
const channel = createMockedChannel(true)
167+
168+
// start again
169+
channel.startReceiveTimeout()
170+
171+
expect(channel._conn.getCalls().setTimeout.length).toEqual(1)
172+
})
173+
})
174+
})
175+
176+
describe('.stopReceiveTimeout()', () => {
177+
describe('when receive timeout is setup', () => {
178+
it ('should not call socket.setTimeout(0) when not started', () => {
179+
const { channel } = setup()
180+
181+
channel.stopReceiveTimeout()
182+
183+
expect(channel._conn.getCalls().setTimeout.length).toEqual(1)
184+
})
185+
186+
it ('should call socket.setTimeout(0) when already started', () => {
187+
const { channel } = setup()
188+
189+
channel.startReceiveTimeout()
190+
191+
channel.stopReceiveTimeout()
192+
193+
expect(channel._conn.getCalls().setTimeout.length).toEqual(3)
194+
expect(channel._conn.getCalls().setTimeout[2]).toEqual([0])
195+
})
196+
197+
it ('should not call socket.setTimeout(0) when already stopped', () => {
198+
const { channel } = setup()
199+
200+
channel.startReceiveTimeout()
201+
channel.stopReceiveTimeout()
202+
203+
channel.stopReceiveTimeout()
204+
205+
expect(channel._conn.getCalls().setTimeout.length).toEqual(3)
206+
})
207+
208+
function setup () {
209+
const channel = createMockedChannel(true)
210+
const receiveTimeout = 42
211+
channel.setupReceiveTimeout(receiveTimeout)
212+
return {channel, receiveTimeout}
213+
}
214+
})
215+
216+
describe('when receive timeout is not setup', () => {
217+
it ('should not call socket.setTimeout(0)', () => {
218+
const channel = createMockedChannel(true)
219+
220+
channel.startReceiveTimeout()
221+
channel.stopReceiveTimeout()
222+
223+
expect(channel._conn.getCalls().setTimeout.length).toEqual(1)
224+
})
225+
})
226+
})
111227
})
112228

113229
function createMockedChannel (connected, config = {}) {

‎packages/bolt-connection/test/connection/connection-channel.test.js

+55-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ describe('ChannelConnection', () => {
6767
[{ hints: { 'connection.recv_timeout_seconds': 0n } }],
6868
[{ hints: { 'connection.recv_timeout_seconds': int(0) } }]
6969
])(
70-
'should call not call this._ch.setupReceiveTimeout() when onComplete metadata is %o',
70+
'should not call this._ch.setupReceiveTimeout() when onComplete metadata is %o',
7171
async metadata => {
7272
const channel = {
7373
setupReceiveTimeout: jest.fn().mockName('setupReceiveTimeout')
@@ -286,6 +286,60 @@ describe('ChannelConnection', () => {
286286
})
287287
})
288288

289+
describe('.__handleOngoingRequestsNumberChange()', () => {
290+
it('should call channel.stopReceiveTimeout when requets number equals to 0', () => {
291+
const channel = {
292+
stopReceiveTimeout: jest.fn().mockName('stopReceiveTimeout'),
293+
startReceiveTimeout: jest.fn().mockName('startReceiveTimeout')
294+
}
295+
const connection = spyOnConnectionChannel({ channel, protocolSupplier: () => undefined })
296+
297+
connection._handleOngoingRequestsNumberChange(0)
298+
299+
expect(channel.stopReceiveTimeout).toHaveBeenCalledTimes(1)
300+
})
301+
302+
it('should not call channel.startReceiveTimeout when requets number equals to 0', () => {
303+
const channel = {
304+
stopReceiveTimeout: jest.fn().mockName('stopReceiveTimeout'),
305+
startReceiveTimeout: jest.fn().mockName('startReceiveTimeout')
306+
}
307+
const connection = spyOnConnectionChannel({ channel, protocolSupplier: () => undefined })
308+
309+
connection._handleOngoingRequestsNumberChange(0)
310+
311+
expect(channel.startReceiveTimeout).toHaveBeenCalledTimes(0)
312+
})
313+
314+
it.each([
315+
[1], [2], [3], [5], [8], [13], [3000]
316+
])('should call channel.startReceiveTimeout when requets number equals to %d', (requests) => {
317+
const channel = {
318+
stopReceiveTimeout: jest.fn().mockName('stopReceiveTimeout'),
319+
startReceiveTimeout: jest.fn().mockName('startReceiveTimeout')
320+
}
321+
const connection = spyOnConnectionChannel({ channel, protocolSupplier: () => undefined })
322+
323+
connection._handleOngoingRequestsNumberChange(requests)
324+
325+
expect(channel.startReceiveTimeout).toHaveBeenCalledTimes(1)
326+
})
327+
328+
it.each([
329+
[1], [2], [3], [5], [8], [13], [3000]
330+
])('should not call channel.stopReceiveTimeout when requets number equals to %d', (requests) => {
331+
const channel = {
332+
stopReceiveTimeout: jest.fn().mockName('stopReceiveTimeout'),
333+
startReceiveTimeout: jest.fn().mockName('startReceiveTimeout')
334+
}
335+
const connection = spyOnConnectionChannel({ channel, protocolSupplier: () => undefined })
336+
337+
connection._handleOngoingRequestsNumberChange(requests)
338+
339+
expect(channel.stopReceiveTimeout).toHaveBeenCalledTimes(0)
340+
})
341+
})
342+
289343
function spyOnConnectionChannel ({
290344
channel,
291345
errorHandler,

‎packages/neo4j-driver/test/internal/dummy-channel.js

+4
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ export default class DummyChannel {
3636
this.written.push(buf)
3737
}
3838

39+
stopReceiveTimeout () {}
40+
41+
startReceiveTimeout () {}
42+
3943
toHex () {
4044
let out = ''
4145
for (let i = 0; i < this.written.length; i++) {

0 commit comments

Comments
 (0)