Add support for a default_radius flag

[whytro]

Issue

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.

Relevant to:

#2983 Require a radius parameter other than unlimited when using bearings

Please read our documentation on release and version management.
If your PR is still work in progress please attach the relevant label.

Tasklist

• CHANGELOG.md entry (How to write a changelog entry)
• update relevant Wiki pages
• add tests (see testing documentation)
• review
• adjust for comments
• cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

Relevant to PR #6572: Require a radius parameter when using bearings (GSoC)

[SiarheiFedartsou]

There is some weird "Too many open files" error in these tests https://github.com/Project-OSRM/osrm-backend/actions/runs/4467036487/jobs/7850020794?pr=6575

I assume that probably it is something which we already had, but didn't know about: each test creates new OSRM object, this object opens files(e.g. to load dataset), but then doesn't close it(or close it, but only when NodeJs GC collects it - tbh I have a bit of lack of knowledge here). So you added a couple of more tests and now we exceed this system threshold. So, may I ask you to check if this is the case? You can just comment out these tests and push - and we will see if problem will disappear? And after that we can come up with what to do with it - probably we can fix that separately.

[SiarheiFedartsou]

https://github.com/nodejs/node-addon-api/blob/main/doc/object_wrap.md

Caution: When the JavaScript object is garbage collected, the call to the C++ destructor may be deferred until a later time. Within that period, Value() will return an empty value.

[whytro]

Okay, I've commented those tests out for now.

[SiarheiFedartsou]

Hey, now I think we are good with this PR in general, but can we try to understand what is wrong with those NodeJs tests failing on macOS?

I guess the reason for it is simple: we close dataset files in desctructors of C++ object and the time when they are called in NodeJs is undetermined. And I think it is okay for production use cases since people don't create multiple OSRM instances. So I would propose trying to increase upper limit for number of file descriptors on CI https://superuser.com/questions/302754/increase-the-maximum-number-of-open-file-descriptors-in-snow-leopard
it probably can be done somewhere here

osrm-backend/.github/workflows/osrm-backend.yml

Line 383 in 192d077

- name: Prepare environment

We can check if it works and if it is not, then just create a ticket to resolve it later and merge this PR. WDYT @whytro ? I think you can uncomment those tests back and I can play with fix I proposed(feel free to do it on your own though, but it won't be really convenient - as you can see github asks for manual approval for each your CI run).

[whytro]

Admittedly, I'm not too familiar on configuring CI workflows, but given the resources you've linked, I've made a minor adjustment in the section mentioned that should hopefully fix it. I've also re-enabled the tests.

[SiarheiFedartsou]

Admittedly, I'm not too familiar on configuring CI workflows,

I didn't mean it would be hard to do for you, I meant it would be not very convenient as each time you would have to wait until I push this button :) But thanks for the try 👍

[whytro]

Oh no worries, I understood you there - I just wanted to mean that I'd given it a try but I'm not too sure if the change I made was correct because of my unfamiliarity with it!

[SiarheiFedartsou]

@whytro many thanks for this contribution 👍 Problem with tests is still there, so I want to understand why it happens (playing with it here #6578) and will update this PR afterwards with fix I will come up with.

[whytro]

@SiarheiFedartsou Thank you for the guidance! Should I close out the other PR (#6572)?

[SiarheiFedartsou]

@SiarheiFedartsou Thank you for the guidance! Should I close out the other PR (#6572)?

I think we can keep it for the future: I think we can merge it on the next major version update.

[whytro]

Okay! I'll also work on refining it, as there are some things in that PR I've noticed while working on this one that I realized I didn't like, or missed. That way, it'll be in better shape for when the next major version update is being prepped.

[nilsnolde]

Thanks for all the work and hanging in there @whytro ! I'd just like to remind you to not forget about the application document for GSoC:) There's still time of course, still wanted to mention it.

[whytro]

Thanks! I've just been reading through the guidelines quite a bit regarding the application - I'll probably drop any remaining questions about the scale/expectations of #6558 after a day or two of some additional preliminary research!

[nilsnolde]

Feel free to get in touch beforehand, @SiarheiFedartsou and me both have a public email address on Github. We could schedule a getting-to-know-each-other meeting if you want. I'd personally actually prefer that. Then we can also let you know what we'd expect in terms of application, so you don't have to spend too much time on it.

[SiarheiFedartsou]

Hey @whytro
I fixed those Node tests: not sure why we had this Too many open files error, but it has gone as soon as I added path to dataset when creating OSRM instance. So now I think we are good to merge it.

Thanks for this contribution and looking forward to work with you on our GSoC project 🚀