-
Notifications
You must be signed in to change notification settings - Fork 0
Hotfix: client-side memory issues #1255
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
Changes from all commits
2ad9770
7790858
0df6005
72345f4
2a24628
76344b0
050490f
d5f01af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { DAS_HOST } from '../constants'; | |
const urlContainsOwnHost = url => url.includes('http'); | ||
const imgIsDataUrl = url => url.includes('data:image'); | ||
const imgIsFromStaticMedia = url => /^(\/static\/media)/.test(url); | ||
const isObjectURL = url => url && typeof url === 'string' && url.startsWith('blob:'); | ||
|
||
const imgNeedsHostAppended = url => { | ||
if (urlContainsOwnHost(url)) return false; | ||
|
@@ -11,42 +12,81 @@ const imgNeedsHostAppended = url => { | |
return true; | ||
}; | ||
|
||
export const imgElFromSrc = (src, width = 30, height = null) => new Promise((resolve, reject) => { | ||
let img = new Image(); | ||
const imageCache = new Map(); | ||
|
||
const generateImageCacheKey = (src, width, height) => { | ||
const w = width === null ? 'null' : (width === undefined ? 'undefined' : width); | ||
const h = height === null ? 'null' : (height === undefined ? 'undefined' : height); | ||
return `${src}:${w}:${h}`; | ||
}; | ||
|
||
export const imgElFromSrc = (src, baseUnit = null) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use more readable method and variable names? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally! Maybe in a separate technical debt task? This is a hotfix branch and these are by and large existing function names |
||
if (!src) { | ||
return Promise.reject('no src provided'); | ||
} | ||
|
||
const cacheKey = generateImageCacheKey(src, baseUnit, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like if (baseUnit && img.naturalWidth && img.naturalHeight) {
const widthIsLarger = img.naturalWidth > img.naturalHeight;
const aspectRatio = widthIsLarger ?
img.naturalHeight / img.naturalWidth :
img.naturalWidth / img.naturalHeight;
if (widthIsLarger) {
img.width = baseUnit;
img.height = Math.round(baseUnit * aspectRatio);
} else {
img.height = baseUnit;
img.width = Math.round(baseUnit * aspectRatio);
}
} else if (baseUnit) {
img.width = baseUnit;
img.height = baseUnit * aspectRatio;
} else {
img.height = baseUnit;
img.width = baseUnit * aspectRatio;
} Does it make sense to update the |
||
|
||
if (imageCache.has(cacheKey)) { | ||
return imageCache.get(cacheKey); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I don't have a full understanding of what is going on, but it feels like this is a patch and not the solution to the issue. Why do we need to cache the images? Shouldn't we instead ask ourselves why is this method being called so many times being asked for the same resources? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The render cycle for the map clusters builds the image strings each time, which is normal since that's how the functional programming nature of React componentry works, and then has traditionally relied on browser caching mechanisms to not over-request images which have already been fetched. Since we're creating object URLs for some images, this causes a memory leak; the object URLs are being created each time, and are held in memory indefinitely. Having our own cache here prevents |
||
} | ||
|
||
const img = new Image(); | ||
img.setAttribute('crossorigin', 'anonymous'); | ||
|
||
img.addEventListener('load', () => { | ||
if (width && height) { | ||
img.width = width; | ||
img.height = height; | ||
} else { | ||
const baseUnit = width || height; | ||
const { naturalHeight, naturalWidth } = img; | ||
const largest = Math.max(naturalHeight, naturalWidth) || baseUnit; | ||
const smallest = Math.min(naturalHeight, naturalWidth) || baseUnit; | ||
const widthIsLarger = largest === naturalWidth; | ||
const aspectRatio = smallest / largest; | ||
if (widthIsLarger) { | ||
const imagePromise = new Promise((resolve, reject) => { | ||
const cleanupAndResolve = () => { | ||
if (baseUnit && img.naturalWidth && img.naturalHeight) { | ||
const widthIsLarger = img.naturalWidth > img.naturalHeight; | ||
|
||
const aspectRatio = widthIsLarger ? | ||
img.naturalHeight / img.naturalWidth : | ||
img.naturalWidth / img.naturalHeight; | ||
|
||
if (widthIsLarger) { | ||
img.width = baseUnit; | ||
img.height = Math.round(baseUnit * aspectRatio); | ||
} else { | ||
img.height = baseUnit; | ||
img.width = Math.round(baseUnit * aspectRatio); | ||
} | ||
} else if (baseUnit) { | ||
img.width = baseUnit; | ||
img.height = baseUnit * aspectRatio; | ||
} else { | ||
img.height = baseUnit; | ||
img.width = baseUnit * aspectRatio; | ||
} | ||
} | ||
resolve(img); | ||
}, { once: true }); | ||
|
||
img.onerror = (e) => { | ||
console.warn('image error', src, e); | ||
reject('could not load image'); | ||
}; | ||
img.src = src; | ||
}); | ||
|
||
|
||
|
||
resolve(img); | ||
}; | ||
|
||
img.addEventListener('load', cleanupAndResolve, { once: true }); | ||
|
||
img.onerror = (e) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice cleanup here |
||
console.warn('image error', src, e); | ||
|
||
if (isObjectURL(src)) { | ||
URL.revokeObjectURL(src); | ||
} | ||
|
||
imageCache.delete(cacheKey); | ||
img.onload = null; | ||
img.onerror = null; | ||
reject('could not load image'); | ||
}; | ||
|
||
img.src = src; | ||
}); | ||
|
||
imageCache.set(cacheKey, imagePromise); | ||
|
||
return imagePromise; | ||
}; | ||
|
||
export const calcImgIdFromUrlForMapImages = (src, width = null, height = null) => { | ||
const path = calcUrlForImage(src); | ||
return `${path}-${width ? width: 'x'}-${height ? height: 'x'}`; | ||
return `${path}-${width ? width : 'x'}-${height ? height : 'x'}`; | ||
}; | ||
|
||
export const calcUrlForImage = imagePath => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,214 @@ | ||
import { imgElFromSrc, calcImgIdFromUrlForMapImages, calcUrlForImage, registerActiveURL } from './img'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the tests!! |
||
|
||
global.URL.createObjectURL = jest.fn(); | ||
global.URL.revokeObjectURL = jest.fn(); | ||
|
||
describe('img utility functions', () => { | ||
describe('calcUrlForImage', () => { | ||
beforeEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
it('returns null for null input', () => { | ||
expect(calcUrlForImage(null)).toBeNull(); | ||
}); | ||
|
||
it('returns the original URL if it already has a host', () => { | ||
const url = 'https://example.com/image.jpg'; | ||
expect(calcUrlForImage(url)).toBe(url); | ||
}); | ||
|
||
it('returns the original URL if it is a data URL', () => { | ||
const url = 'data:image/png;base64,abc123'; | ||
expect(calcUrlForImage(url)).toBe(url); | ||
}); | ||
|
||
it('returns the original URL if it is from static media', () => { | ||
const url = '/static/media/image.jpg'; | ||
const cleaned = calcUrlForImage(url); | ||
|
||
expect(cleaned).toBe(url); | ||
}); | ||
|
||
it('appends host to URL that needs it', () => { | ||
const url = 'images/test.jpg'; | ||
const cleaned = calcUrlForImage(url); | ||
expect(cleaned).toBe(`https://localhost/${url}`); | ||
}); | ||
}); | ||
|
||
describe('calcImgIdFromUrlForMapImages', () => { | ||
it('calculates image ID with just src', () => { | ||
const src = 'images/test.jpg'; | ||
const expectedUrl = calcUrlForImage(src); | ||
expect(calcImgIdFromUrlForMapImages(src)).toBe(`${expectedUrl}-x-x`); | ||
}); | ||
|
||
it('calculates image ID with src and width', () => { | ||
const src = 'images/test.jpg'; | ||
const width = 100; | ||
const expectedUrl = calcUrlForImage(src); | ||
expect(calcImgIdFromUrlForMapImages(src, width)).toBe(`${expectedUrl}-${width}-x`); | ||
}); | ||
|
||
it('calculates image ID with src, width, and height', () => { | ||
const src = 'images/test.jpg'; | ||
const width = 100; | ||
const height = 200; | ||
const expectedUrl = calcUrlForImage(src); | ||
expect(calcImgIdFromUrlForMapImages(src, width, height)).toBe(`${expectedUrl}-${width}-${height}`); | ||
}); | ||
}); | ||
|
||
describe('imgElFromSrc', () => { | ||
let mockImage; | ||
let loadCallback; | ||
|
||
beforeEach(() => { | ||
|
||
mockImage = { | ||
setAttribute: jest.fn(), | ||
addEventListener: jest.fn((event, callback) => { | ||
if (event === 'load') loadCallback = callback; | ||
}), | ||
onerror: jest.fn(), | ||
naturalWidth: 100, | ||
naturalHeight: 50, | ||
width: 0, | ||
height: 0, | ||
src: '', | ||
}; | ||
|
||
global.Image = jest.fn(() => mockImage); | ||
global.URL.revokeObjectURL.mockClear(); | ||
}); | ||
|
||
it('creates an image element with crossorigin attribute', async () => { | ||
const loadPromise = imgElFromSrc('test.jpg'); | ||
expect(mockImage.setAttribute).toHaveBeenCalledWith('crossorigin', 'anonymous'); | ||
|
||
|
||
loadCallback(); | ||
await loadPromise; | ||
}); | ||
|
||
it('sets width and height based on baseUnit', async () => { | ||
const loadPromise = imgElFromSrc('test.jpg', 200); | ||
|
||
loadCallback(); | ||
const img = await loadPromise; | ||
|
||
expect(img.width).toBe(200); | ||
expect(img.height).toBe(100); | ||
}); | ||
|
||
it('calculates dimensions based on width if only width is provided', async () => { | ||
mockImage.naturalWidth = 200; | ||
mockImage.naturalHeight = 100; | ||
|
||
const loadPromise = imgElFromSrc('test.jpg', 50); | ||
|
||
|
||
loadCallback(); | ||
const img = await loadPromise; | ||
|
||
expect(img.width).toBe(50); | ||
expect(img.height).toBe(25); | ||
}); | ||
|
||
it('handles case when natural dimensions are not available', async () => { | ||
const uniqueSrc = `test-no-dimensions-${Date.now()}.jpg`; | ||
|
||
mockImage.naturalWidth = 0; | ||
mockImage.naturalHeight = 0; | ||
|
||
const loadPromise = imgElFromSrc(uniqueSrc, 50); | ||
|
||
loadCallback(); | ||
const img = await loadPromise; | ||
|
||
expect(img.width).toBe(50); | ||
expect(img.height).toBe(50); | ||
}); | ||
|
||
it('revokes object URLs for images that fail to load', async () => { | ||
const testSrc = 'blob:https://example.com/image-error.jpg'; | ||
global.URL.revokeObjectURL.mockClear(); | ||
|
||
imgElFromSrc(testSrc, 50).catch(() => console.info('caught the error as i should')); | ||
|
||
expect(mockImage.src).toBe(testSrc); | ||
|
||
mockImage.onerror(new Error('image loading failed')); | ||
|
||
expect(global.URL.revokeObjectURL).toHaveBeenCalledWith(testSrc); | ||
}); | ||
|
||
describe('memoization', () => { | ||
it('returns cached promise for identical requests', async () => { | ||
const src = 'https://example.com/image.jpg'; | ||
const width = 50; | ||
const promise1 = imgElFromSrc(src, width); | ||
const promise2 = imgElFromSrc(src, width); | ||
|
||
expect(promise1).toBe(promise2); | ||
|
||
loadCallback(); | ||
await promise1; | ||
}); | ||
|
||
it('creates new promise for different image sources', async () => { | ||
const promise1 = imgElFromSrc('image1.jpg', 50); | ||
const promise2 = imgElFromSrc('image2.jpg', 50); | ||
|
||
expect(promise1).not.toBe(promise2); | ||
|
||
loadCallback(); | ||
|
||
const secondImage = global.Image.mock.results[1].value; | ||
const secondLoadCallback = secondImage.addEventListener.mock.calls.find( | ||
call => call[0] === 'load' | ||
)[1]; | ||
secondLoadCallback(); | ||
|
||
await Promise.all([promise1, promise2]); | ||
}); | ||
|
||
it('creates new promise for same source but different dimensions', async () => { | ||
const src = 'image.jpg'; | ||
const promise1 = imgElFromSrc(src, 50); | ||
const promise2 = imgElFromSrc(src, 100); | ||
|
||
expect(promise1).not.toBe(promise2); | ||
|
||
loadCallback(); | ||
|
||
const secondImage = global.Image.mock.results[1].value; | ||
const secondLoadCallback = secondImage.addEventListener.mock.calls.find( | ||
call => call[0] === 'load' | ||
)[1]; | ||
secondLoadCallback(); | ||
|
||
await Promise.all([promise1, promise2]); | ||
}); | ||
|
||
it('removes failed requests from cache', async () => { | ||
const mapDeleteSpy = jest.spyOn(Map.prototype, 'delete'); | ||
|
||
const loadPromise = imgElFromSrc('broken-image.jpg', 50); | ||
|
||
mockImage.onerror(new Error('test error')); | ||
|
||
try { | ||
await loadPromise; | ||
} catch (e) { | ||
|
||
} | ||
|
||
expect(mapDeleteSpy).toHaveBeenCalled(); | ||
|
||
mapDeleteSpy.mockRestore(); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a global variable as a cache store has a smell to me. Maybe it's time to re-work the approach of these methods? Couldn't we have a hook with a state variable with the "cache" that passes down through context the
imgElFromSrc
method? Something like that sounds like the React approach, easier to memoize, debug, etc...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's less efficient to do it that way, but we can certainly React-ify it more if you would like. I would prefer just using a factory function to create a nice closure to wrap the cache in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it shouldn't be less efficient if it's properly memoized. Maybe we are talking about a couple renders and a couple milliseconds, but React re-renders hundreds of times and the cache mechanism will make those milliseconds be negligible. I think the React approach is a better fit four our React application, but I also understand it's out of the scope of a hotfix, just wanted to share the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often and how much data can/should be held by this cache? Would a regular js map be good enough for a cache? Or should we use like a bounded in-memory cache to avoid another memory leak - the cache can potentially grow indefinitely here.
Also, we could reach for persistent solutions like LocalStorage/SessionStorage.