search

iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
601 stars 210 forks source link

Race condition in SettingsClient getUrl #2305

Closed alarkbentley closed 2 years ago

alarkbentley commented 2 years ago

I found an ugly race condition in the getUrl because it caches the returned result before the excludeApiVersion is considered. I made two asynchronous requests for getUrl it first returned the URL without ApiVersion and later with ApiVersion as the first request has been processed.

Also it is impossible to later on change the excludeApiVersion flag as the results is cached without it in consideration.

Will need this issue fix backported to 2.x

https://github.com/imodeljs/imodeljs/blob/a3f39c8e4fb80774c4943da2af6923d8f91b289e/clients/product-settings/src/SettingsClient.ts#L47-L57

pmconne commented 2 years ago

@calebmshafer this code is going away, correct? Does that mean this bug will also go away?

alarkbentley commented 2 years ago

OCP is using this code today however so a fix would be appreciated. I have to await all calls that depend on getUrl as a workaround for this to not be an issue.

calebmshafer commented 2 years ago

@alarkbentley this issue is fixed in the base Client class but doesn't work since the SettingsClient is overriding the getUrl method. It should be a fairly easy fix. I'll submit a PR to 2.19.x and get that into a patch by beginning of next week.

pmconne commented 2 years ago

@calebmshafer has that fix been released for 2.19.x?

aruniverse commented 2 years ago

should be fixed in 2.19.17