Timeout for OSRM && headers by yanngv29 · Pull Request #10 · Project-OSRM/osrm.js

yanngv29 · 2021-05-27T08:22:17Z

add timeout case when calling OSRM
add headers capabilities when, for example, you need to pass some credentials to the OSRM server

some dependencies update.

…k call

Copilot

Pull request overview

Adds support for request headers and improves timeout handling when calling an OSRM server, along with dependency/build updates.

Changes:

Add configurable default headers (headers) on the OSRM client and forward them into HTTP(S) requests.
Rework request execution to use http(s).request() with timeout handling.
Update package version/dependencies and adjust tests/build scripts.

Reviewed changes

Copilot reviewed 3 out of 7 changed files in this pull request and generated 9 comments.

File	Description
`src/osrm.js`	Adds `headers` support and refactors HTTP request creation + timeout handling.
`test/test.js`	Updates expected match coordinates and adds debug logging in the tile test.
`package.json`	Bumps version/dev deps and changes the build (`prepub`) script.
`package-lock.json`	Introduces lockfile reflecting updated dependency graph.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T18:51:42Z

src/osrm.js

+  this._get = function(url, callback,timeoutCb) {
+    var parsedUrl = Url.parse(url);
+    var options = {
+      protocol : parsedUrl.protocol,
+      hostname : parsedUrl.hostname,
+      port : parsedUrl.port,
+      path : parsedUrl.path,
+      headers : this._headers,
+      timeout : this._timeout
+    };
    if (protocol === "http:")
    {
-      return http.get(url, callback);
+      var clientRequest = http.request(options, callback);
+      clientRequest.setTimeout(this._timeout,timeoutCb);
+      return clientRequest;
    }
    else if (protocol == "https:")
    {
-      return https.get(url, callback);
+      var clientRequest = https.request(options, callback);
+      clientRequest.setTimeout(this._timeout,timeoutCb);
+      return clientRequest;


request.setTimeout(...) is being registered twice: once inside _get via clientRequest.setTimeout(this._timeout, timeoutCb) and again in request() via .setTimeout(this._timeout, ...). On a socket timeout, both listeners can fire and invoke the user callback twice, and the _get timeout handler also doesn’t abort/destroy the request. Consider consolidating timeout handling in one place (preferably in request()), ensuring the request is destroyed/aborted, and guarding all completion paths so callback can only be called once.

Copilot · 2026-04-01T18:51:42Z

src/osrm.js

        }
      });
+    },function () {
+      console.error("osrm TIMEOUT detected -> returning an error");


The timeout callback passed to _get logs to stderr (console.error(...)). Since this is library code, this will produce unexpected noisy output for consumers and can’t be controlled. Prefer returning the timeout error via the callback only, and let the caller decide how/where to log.

Suggested change

console.error("osrm TIMEOUT detected -> returning an error");

Copilot · 2026-04-01T18:51:42Z

src/osrm.js

    var timedout;
    var request = this._get(url, function (response) {
      var body = '';
+
      response.on('data', function(data) {
        body += data;
      });


Timeout handling now sets timedout = true and calls callback, but the response handlers later in request() don’t check timedout before calling callback (e.g., in the response 'end' path). This can lead to the callback being invoked more than once if the server responds after the timeout. Add a single “done”/once guard that all success/error/timeout paths use before invoking the callback.

Copilot · 2026-04-01T18:51:42Z

src/osrm.js

+  this._get = function(url, callback,timeoutCb) {
+    var parsedUrl = Url.parse(url);
+    var options = {
+      protocol : parsedUrl.protocol,
+      hostname : parsedUrl.hostname,
+      port : parsedUrl.port,
+      path : parsedUrl.path,
+      headers : this._headers,
+      timeout : this._timeout
+    };
    if (protocol === "http:")
    {
-      return http.get(url, callback);
+      var clientRequest = http.request(options, callback);
+      clientRequest.setTimeout(this._timeout,timeoutCb);
+      return clientRequest;
    }
    else if (protocol == "https:")
    {


_get builds options from parsedUrl, but selects the HTTP client (http vs https) based on the outer protocol parsed from this._url, not parsedUrl.protocol. If a full URL with a different protocol is ever passed in (or this._url changes), this can send requests with the wrong module. Prefer branching on parsedUrl.protocol (or options.protocol) to match the request being made.

Copilot · 2026-04-01T18:51:43Z

src/osrm.js

      this._url = arg.url || this._url;
      this._profile = arg.profile || this._profile;
      this._timeout = arg.timeout || this._timeout;
+      this._headers = arg.headers || this._headers;


Option defaults in the constructor use || (e.g., this._timeout = arg.timeout || this._timeout). This prevents callers from intentionally setting falsy values like timeout: 0 (often used to disable timeouts) or headers: {} vs null. Consider using an explicit undefined check (e.g., if ('timeout' in arg) ...) so valid falsy values can be provided.

Suggested change

this._url = arg.url || this._url;

this._profile = arg.profile || this._profile;

this._timeout = arg.timeout || this._timeout;

this._headers = arg.headers || this._headers;

if (arg.url !== undefined) this._url = arg.url;

if (arg.profile !== undefined) this._profile = arg.profile;

if (arg.timeout !== undefined) this._timeout = arg.timeout;

if (arg.headers !== undefined) this._headers = arg.headers;

Copilot · 2026-04-01T18:51:43Z

src/osrm.js

    }).setTimeout(this._timeout, function() {
      request.abort();
      timedout = true;
      callback(new Error("Request timed out"));
    });


The timeout handler uses request.abort(). In modern Node.js this is deprecated in favor of request.destroy() (and abort() has slightly different semantics/events). Consider switching to destroy() and ensuring the timeout error path can only fire once (especially given the additional timeout listener added in _get).

Copilot · 2026-04-01T18:51:43Z

test/test.js

+    console.log("ratio : "+ ratio);
+    console.log("result  : "+JSON.stringify(result));


The tile test now logs ratio and the full result buffer to stdout. This will add noisy output to normal test runs and CI logs, and JSON.stringify(result) can be very large. Please remove these console.log lines (or gate them behind an explicit debug flag if needed).

Suggested change

console.log("ratio : "+ ratio);

console.log("result : "+JSON.stringify(result));

Copilot · 2026-04-01T18:51:43Z

package.json

  },
  "scripts": {
-    "prepub": "browserify -d src/osrm.js -s osrm > dist/osrm.raw.js && uglifyjs dist/osrm.raw.js -c -m --source-map=dist/osrm.js.map -o dist/osrm.js",
+    "prepub": "browserify -d src/osrm.js -s osrm > dist/osrm.raw.js && uglifyjs dist/osrm.raw.js -c -m -o dist/osrm.js",


prepub no longer generates a source map, but the repo currently contains dist/osrm.js.map. This can leave the committed map stale/out-of-sync (or missing after running the script). Either keep generating the map in prepub or remove/update the committed .map artifact so dist/ stays consistent.

Suggested change

"prepub": "browserify -d src/osrm.js -s osrm > dist/osrm.raw.js && uglifyjs dist/osrm.raw.js -c -m -o dist/osrm.js",

"prepub": "browserify -d src/osrm.js -s osrm > dist/osrm.raw.js && uglifyjs dist/osrm.raw.js -c -m --source-map dist/osrm.js.map -o dist/osrm.js",

Copilot · 2026-04-01T18:51:44Z

src/osrm.js

  this._url = 'https://router.project-osrm.org';
  this._profile = 'driving';
  this._timeout = 10000; // 10 seconds
+  this._headers = {};



New behavior for request headers (this._headers forwarded into request options) and timeout handling is not covered by tests. Consider adding tests that spin up a local HTTP server to (1) assert incoming headers match arg.headers and (2) delay responses to assert the callback receives a timeout error and is invoked only once.

yanngv29 and others added 15 commits April 26, 2018 16:15

Adding headers in options to send headers with the request

20d3006

bug correction : do not clear timeout on error : cause double callbac…

df34e5e

…k call

bug correction : do not clear timeout on error : cause double callbac…

04a7ec9

…k call

generation dist file

996073f

evol version 5.6.0

339120b

using http(s).request .setTimeout to timeout the request

c0e16e2

using http(s).request .setTimeout to timeout the request

b3a9e20

last bug correcting

e4a1899

last bug correcting

68b1bc4

last bug correcting

8c76d97

patch double callback call if timeout

cfe8e17

npm audit fix force and prepub actions

3765836

merge with source master

5a418f5

update package-lock.json

ecb9938

correctif isTimeout

046dfa8

DennisOSRM requested a review from Copilot April 1, 2026 18:47

Copilot started reviewing on behalf of DennisOSRM April 1, 2026 18:48 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeout for OSRM && headers#10

Timeout for OSRM && headers#10
yanngv29 wants to merge 15 commits intoProject-OSRM:masterfrom
yanngv29:master

yanngv29 commented May 27, 2021 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		console.log("ratio : "+ ratio);
		console.log("result : "+JSON.stringify(result));

	"prepub": "browserify -d src/osrm.js -s osrm > dist/osrm.raw.js && uglifyjs dist/osrm.raw.js -c -m -o dist/osrm.js",
	"prepub": "browserify -d src/osrm.js -s osrm > dist/osrm.raw.js && uglifyjs dist/osrm.raw.js -c -m --source-map dist/osrm.js.map -o dist/osrm.js",

Conversation

yanngv29 commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yanngv29 commented May 27, 2021 •

edited

Loading