Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(mtx): sidecar scenario due to usage of wrong credentials #732

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

swaldmann
Copy link
Contributor

@swaldmann swaldmann commented Jul 8, 2024

We currently have a out of memory in @cap-js/hana that doesn't occur with @sap/cds-hana:

# In cap/dev:
cf login --sso
cf create-service service-manager container bookshop-sm
cds init bookshop --add multitenancy
cd bookshop/mtx/sidecar
cds bind -2 bookshop-sm --for hana
cds run --profile hana --resolve-bindings
BOOM 💣 → out of memory

To get heap snapshots run this instead of plain cds run:

NODE_OPTIONS='--heapsnapshot-near-heap-limit=3 --max-old-space-size=100' cds run --profile hana --resolve-bindings

Turns out the memory is full of type errors from hdb:
Screenshot 2024-07-08 at 12 29 00

From the error I could tell it tried to open the hdb connection with Service Manger credentials instead of the actual credentials fetched from it.

The problem is that in a sidecar scenario we don't explicitly set cds.requires.multitenancy (might change, see also internally cap/cds-mtxs/pull/1006), so @cap-js/hana takes a wrong turn.

I also found out the create function here is called extremely often in succession in the error case, which explains why the memory is filled up so quickly.

This PR fixes the issue for our sidecar setup and older MTX versions (if we also decide to go with cap/cds-mtxs/pull/1006) but we should still investigate in a follow-up how the behavior can be improved so the create isn't called so often without debouncing and filling up the memory in the error case.

@swaldmann swaldmann changed the title Fix memory leak in MTX sidecar scenario fix: memory leak in MTX sidecar scenario Jul 8, 2024
@johannes-vogel
Copy link
Contributor

@swaldmann Do you know what was different in old db impl? To me it seems pretty much the same logic for mt detection.

@swaldmann swaldmann changed the title fix: memory leak in MTX sidecar scenario fix: memory overflow due to hanging in MTX sidecar scenario Jul 8, 2024
@swaldmann
Copy link
Contributor Author

Discussed in DB layer sync -- we'll take this impl as it works for older versions of cds-mtxs and the sm_url is the appropriate field to check anyway

@swaldmann swaldmann enabled auto-merge (squash) July 8, 2024 14:33
@swaldmann swaldmann disabled auto-merge July 8, 2024 14:41
@johannes-vogel johannes-vogel changed the title fix: memory overflow due to hanging in MTX sidecar scenario fix(mtx): sidecar scenario due to usage of wrong credentials Jul 8, 2024
@johannes-vogel johannes-vogel enabled auto-merge (squash) July 8, 2024 14:41
@johannes-vogel johannes-vogel merged commit 0b5c91f into main Jul 8, 2024
7 checks passed
@johannes-vogel johannes-vogel deleted the fix-memory-leak branch July 8, 2024 14:49
@cap-bots cap-bots mentioned this pull request Jul 8, 2024
johannes-vogel added a commit that referenced this pull request Jul 8, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>db-service: 1.11.0</summary>

##
[1.11.0](db-service-v1.10.3...db-service-v1.11.0)
(2024-07-08)


### Added

* **search:** enable deep search with path expressions
([#590](#590))
([e9e9461](e9e9461))

### Changed

* `search` interprets only first search term instead of raising an error
([#707](#707))
([0b9108c](0b9108c))

### Fixed

* optimize foreign key access for expand with aggregations
([#734](#734))
([77b7978](77b7978))
</details>

<details><summary>hana: 1.1.0</summary>

##
[1.1.0](hana-v1.0.1...hana-v1.1.0)
(2024-07-08)


### Added

* Enable native HANA fuzzy search for `search` function queries
([#707](#707))
([0b9108c](0b9108c))


### Fixed

* **mtx:** sidecar scenario due to usage of wrong credentials
([#732](#732))
([0b5c91f](0b5c91f))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: Johannes Vogel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants