[TT-5477] Fix/jsvm memory usage #4249
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
gateway/api_definition.go
Outdated
| @@ -319,6 +326,16 @@ | |||
|
|
|||
| spec.APIDefinition = def | |||
|
|
|||
| apiString, err := json.Marshal(def) | |||
| if err != nil { | |||
| logger.WithError(err).WithField("name", def.Name).Error("Failed to JSON marshal API definition") | |||
Check failure
Code scanning / CodeQL
Log entries created from user input
gateway/reverse_proxy_test.go
Outdated
| defer ts.Close() | ||
| defer ts.MockHandle.ShutdownDnsMock() |
There was a problem hiding this comment.
If you see dnsmock being used somewhere and you're touching that part of the code, just remove it. There's no safe way to use the dns mock (particularly modifying net.DefaultResolver), and we have various utilities that can avoid it in the /test pkg (net.dialer, http client, etc.). Can this be avoided?
jeffy-mathew
force-pushed
the
fix/jsvm-memory-usage
branch
from
ee95123
to
31c86f8
Compare
* remove gw.started flag - out of scope from PR * use mutex lock instead of RLock while removing APIs from gw. * cast assertion for chainObject in serveHTTP
jeffy-mathew
force-pushed
the
fix/jsvm-memory-usage
branch
from
8062225
to
fae61d7
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
API tests result: success ✅ |
|
/release to release-4 |
|
/release to release-4-lts |
|
Working on it! Note that it can take a few minutes. |
1 similar comment
|
Working on it! Note that it can take a few minutes. |
* Ensure that JSVM initialized only if plugin is JSVM or there is virtual endpoint In past it was initialized if ANY plugin is enabled. Now it checks the driver. * Ensure that only APIs which changed will be reloaded If APi checksum has not chaned, re-use and not reload it. Additionally added logic for JSVM to release holding pointers to Gateway and Spec, so it will not have issues with being garbage collected Co-authored-by: sredny buitrago <sredny.buitrago@gmail.com> Co-authored-by: jeff <jeffy.mathew100@gmail.com> (cherry picked from commit 648bd90)
|
@jeffy-mathew Succesfully merged |
|
@jeffy-mathew Seems like there is conflict and it require manual merge. |
|
API tests result: success ✅
In past it was initialized if ANY plugin is enabled. Now it checks the driver.
If APi checksum has not chaned, re-use and not reload it. Co-authored-by: sredny buitrago sredny.buitrago@gmail.com |
* Ensure that JSVM initialized only if plugin is JSVM or there is virtual endpoint In past it was initialized if ANY plugin is enabled. Now it checks the driver. * Ensure that only APIs which changed will be reloaded If APi checksum has not chaned, re-use and not reload it. Additionally added logic for JSVM to release holding pointers to Gateway and Spec, so it will not have issues with being garbage collected Co-authored-by: sredny buitrago <sredny.buitrago@gmail.com> Co-authored-by: jeff <jeffy.mathew100@gmail.com>
|
/release to release-4.2 |
|
Working on it! Note that it can take a few minutes. |
|
/release to release-4.2.2 |
|
Working on it! Note that it can take a few minutes. |
|
@jeffy-mathew Seems like there is conflict and it require manual merge. |
1 similar comment
|
@jeffy-mathew Seems like there is conflict and it require manual merge. |
* Ensure that JSVM initialized only if plugin is JSVM or there is virtual endpoint In past it was initialized if ANY plugin is enabled. Now it checks the driver. * Ensure that only APIs which changed will be reloaded If APi checksum has not chaned, re-use and not reload it. Additionally added logic for JSVM to release holding pointers to Gateway and Spec, so it will not have issues with being garbage collected Co-authored-by: sredny buitrago <sredny.buitrago@gmail.com> Co-authored-by: jeff <jeffy.mathew100@gmail.com>
* Ensure that JSVM initialized only if plugin is JSVM or there is virtual endpoint In past it was initialized if ANY plugin is enabled. Now it checks the driver. * Ensure that only APIs which changed will be reloaded If APi checksum has not chaned, re-use and not reload it. Additionally added logic for JSVM to release holding pointers to Gateway and Spec, so it will not have issues with being garbage collected Co-authored-by: sredny buitrago <sredny.buitrago@gmail.com> Co-authored-by: jeff <jeffy.mathew100@gmail.com>
jeffy-mathew
changed the title
* Ensure that JSVM initialized only if plugin is JSVM or there is virtual endpoint In past it was initialized if ANY plugin is enabled. Now it checks the driver. * Ensure that only APIs which changed will be reloaded If APi checksum has not chaned, re-use and not reload it. Additionally added logic for JSVM to release holding pointers to Gateway and Spec, so it will not have issues with being garbage collected Co-authored-by: sredny buitrago <sredny.buitrago@gmail.com> Co-authored-by: jeff <jeffy.mathew100@gmail.com>
This is an alternative implementation of https://github.com/TykTechnologies/tyk/pull/4215/files
Description
Reloading APIs, which use JSVM, cause gateway memory growth, and eventual crash.
Initialise JSVM only when needed
First of all I found that JSVM get initialized when ANY plugin is used. So first fix is:
Initialise JSVM only when VirtualEndpoint used, or plugin with JSVM engine is used.
It was done by removing initialization code from here https://github.com/TykTechnologies/tyk/compare/fix/jsvm-memory-usage?expand=1#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bL337-L354
And putting it after we already know plugin type https://github.com/TykTechnologies/tyk/compare/fix/jsvm-memory-usage?expand=1#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68R233-R237
Releasing pointers to easy GC
JSVM code by itself, is "a bit of" circular pointer hell. Trying to fix it leads to huge rewrite (and some parts really non obvious on how to do without having links to spec/gateway), which I do not want do as part of this PR. So now it cleanups pointers inside JSVM object, before API gets reloaded, to make GC job easier https://github.com/TykTechnologies/tyk/compare/fix/jsvm-memory-usage?expand=1#diff-78cd278aba997558b7daa7897051a794ef860076d45c93be792791db39381ca0R374-R380
Reloading only APIs which actually changed
For each API we now calculate SHA256
checksum, and if during API reload event, API has not changed, re-use already loaded API and its resources instead. https://github.com/TykTechnologies/tyk/compare/fix/jsvm-memory-usage?expand=1#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68R886-R898How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
fork, don't request your
master!masterbranch (left side). Also, you should startyour branch off our latest
master.go mod tidy && go mod vendorgo fmt -sgo vet