Skip to content

Commit 78857c6

Browse files
committed
Polish Memory Leak Mitigation
Issue gh-9841
1 parent 809ff88 commit 78857c6

File tree

6 files changed

+141
-69
lines changed

6 files changed

+141
-69
lines changed

config/src/main/java/org/springframework/security/config/annotation/web/configuration/SecurityReactorContextConfiguration.java

Lines changed: 141 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2020 the original author or authors.
2+
* Copyright 2002-2021 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,10 +16,14 @@
1616

1717
package org.springframework.security.config.annotation.web.configuration;
1818

19+
import java.util.Collection;
1920
import java.util.Collections;
2021
import java.util.HashMap;
2122
import java.util.Map;
23+
import java.util.Set;
24+
import java.util.concurrent.ConcurrentHashMap;
2225
import java.util.function.Function;
26+
import java.util.function.Supplier;
2327

2428
import jakarta.servlet.http.HttpServletRequest;
2529
import jakarta.servlet.http.HttpServletResponse;
@@ -36,7 +40,6 @@
3640
import org.springframework.context.annotation.Bean;
3741
import org.springframework.context.annotation.Configuration;
3842
import org.springframework.security.core.Authentication;
39-
import org.springframework.security.core.context.SecurityContext;
4043
import org.springframework.security.core.context.SecurityContextHolder;
4144
import org.springframework.web.context.request.RequestAttributes;
4245
import org.springframework.web.context.request.RequestContextHolder;
@@ -68,17 +71,22 @@ static class SecurityReactorContextSubscriberRegistrar implements InitializingBe
6871

6972
private static final String SECURITY_REACTOR_CONTEXT_OPERATOR_KEY = "org.springframework.security.SECURITY_REACTOR_CONTEXT_OPERATOR";
7073

74+
private static final Map<Object, Supplier<Object>> CONTEXT_ATTRIBUTE_VALUE_LOADERS = new HashMap<>();
75+
76+
static {
77+
CONTEXT_ATTRIBUTE_VALUE_LOADERS.put(HttpServletRequest.class,
78+
SecurityReactorContextSubscriberRegistrar::getRequest);
79+
CONTEXT_ATTRIBUTE_VALUE_LOADERS.put(HttpServletResponse.class,
80+
SecurityReactorContextSubscriberRegistrar::getResponse);
81+
CONTEXT_ATTRIBUTE_VALUE_LOADERS.put(Authentication.class,
82+
SecurityReactorContextSubscriberRegistrar::getAuthentication);
83+
}
84+
7185
@Override
7286
public void afterPropertiesSet() throws Exception {
7387
Function<? super Publisher<Object>, ? extends Publisher<Object>> lifter = Operators
7488
.liftPublisher((pub, sub) -> createSubscriberIfNecessary(sub));
75-
Hooks.onLastOperator(SECURITY_REACTOR_CONTEXT_OPERATOR_KEY, (pub) -> {
76-
if (!contextAttributesAvailable()) {
77-
// No need to decorate so return original Publisher
78-
return pub;
79-
}
80-
return lifter.apply(pub);
81-
});
89+
Hooks.onLastOperator(SECURITY_REACTOR_CONTEXT_OPERATOR_KEY, lifter::apply);
8290
}
8391

8492
@Override
@@ -94,45 +102,30 @@ <T> CoreSubscriber<T> createSubscriberIfNecessary(CoreSubscriber<T> delegate) {
94102
return new SecurityReactorContextSubscriber<>(delegate, getContextAttributes());
95103
}
96104

97-
private static boolean contextAttributesAvailable() {
98-
SecurityContext context = SecurityContextHolder.peekContext();
99-
Authentication authentication = null;
100-
if (context != null) {
101-
authentication = context.getAuthentication();
102-
}
103-
return authentication != null
104-
|| RequestContextHolder.getRequestAttributes() instanceof ServletRequestAttributes;
105+
private static Map<Object, Object> getContextAttributes() {
106+
return new LoadingMap<>(CONTEXT_ATTRIBUTE_VALUE_LOADERS);
105107
}
106108

107-
private static Map<Object, Object> getContextAttributes() {
108-
HttpServletRequest servletRequest = null;
109-
HttpServletResponse servletResponse = null;
109+
private static HttpServletRequest getRequest() {
110110
RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
111111
if (requestAttributes instanceof ServletRequestAttributes) {
112112
ServletRequestAttributes servletRequestAttributes = (ServletRequestAttributes) requestAttributes;
113-
servletRequest = servletRequestAttributes.getRequest();
114-
servletResponse = servletRequestAttributes.getResponse(); // possible null
115-
}
116-
SecurityContext context = SecurityContextHolder.peekContext();
117-
Authentication authentication = null;
118-
if (context != null) {
119-
authentication = context.getAuthentication();
120-
}
121-
if (authentication == null && servletRequest == null) {
122-
return Collections.emptyMap();
123-
}
124-
Map<Object, Object> contextAttributes = new HashMap<>();
125-
if (servletRequest != null) {
126-
contextAttributes.put(HttpServletRequest.class, servletRequest);
127-
}
128-
if (servletResponse != null) {
129-
contextAttributes.put(HttpServletResponse.class, servletResponse);
113+
return servletRequestAttributes.getRequest();
130114
}
131-
if (authentication != null) {
132-
contextAttributes.put(Authentication.class, authentication);
115+
return null;
116+
}
117+
118+
private static HttpServletResponse getResponse() {
119+
RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
120+
if (requestAttributes instanceof ServletRequestAttributes) {
121+
ServletRequestAttributes servletRequestAttributes = (ServletRequestAttributes) requestAttributes;
122+
return servletRequestAttributes.getResponse(); // possible null
133123
}
124+
return null;
125+
}
134126

135-
return contextAttributes;
127+
private static Authentication getAuthentication() {
128+
return SecurityContextHolder.getContext().getAuthentication();
136129
}
137130

138131
}
@@ -185,4 +178,112 @@ public void onComplete() {
185178

186179
}
187180

181+
/**
182+
* A map that computes each value when {@link #get} is invoked
183+
*/
184+
static class LoadingMap<K, V> implements Map<K, V> {
185+
186+
private final Map<K, V> loaded = new ConcurrentHashMap<>();
187+
188+
private final Map<K, Supplier<V>> loaders;
189+
190+
LoadingMap(Map<K, Supplier<V>> loaders) {
191+
this.loaders = Collections.unmodifiableMap(new HashMap<>(loaders));
192+
}
193+
194+
@Override
195+
public int size() {
196+
return this.loaders.size();
197+
}
198+
199+
@Override
200+
public boolean isEmpty() {
201+
return this.loaders.isEmpty();
202+
}
203+
204+
@Override
205+
public boolean containsKey(Object key) {
206+
return this.loaders.containsKey(key);
207+
}
208+
209+
@Override
210+
public Set<K> keySet() {
211+
return this.loaders.keySet();
212+
}
213+
214+
@Override
215+
public V get(Object key) {
216+
if (!this.loaders.containsKey(key)) {
217+
throw new IllegalArgumentException(
218+
"This map only supports the following keys: " + this.loaders.keySet());
219+
}
220+
return this.loaded.computeIfAbsent((K) key, (k) -> this.loaders.get(k).get());
221+
}
222+
223+
@Override
224+
public V put(K key, V value) {
225+
if (!this.loaders.containsKey(key)) {
226+
throw new IllegalArgumentException(
227+
"This map only supports the following keys: " + this.loaders.keySet());
228+
}
229+
return this.loaded.put(key, value);
230+
}
231+
232+
@Override
233+
public V remove(Object key) {
234+
if (!this.loaders.containsKey(key)) {
235+
throw new IllegalArgumentException(
236+
"This map only supports the following keys: " + this.loaders.keySet());
237+
}
238+
return this.loaded.remove(key);
239+
}
240+
241+
@Override
242+
public void putAll(Map<? extends K, ? extends V> m) {
243+
for (Map.Entry<? extends K, ? extends V> entry : m.entrySet()) {
244+
put(entry.getKey(), entry.getValue());
245+
}
246+
}
247+
248+
@Override
249+
public void clear() {
250+
this.loaded.clear();
251+
}
252+
253+
@Override
254+
public boolean containsValue(Object value) {
255+
return this.loaded.containsValue(value);
256+
}
257+
258+
@Override
259+
public Collection<V> values() {
260+
return this.loaded.values();
261+
}
262+
263+
@Override
264+
public Set<Entry<K, V>> entrySet() {
265+
return this.loaded.entrySet();
266+
}
267+
268+
@Override
269+
public boolean equals(Object o) {
270+
if (this == o) {
271+
return true;
272+
}
273+
if (o == null || getClass() != o.getClass()) {
274+
return false;
275+
}
276+
277+
LoadingMap<?, ?> that = (LoadingMap<?, ?>) o;
278+
279+
return this.loaded.equals(that.loaded);
280+
}
281+
282+
@Override
283+
public int hashCode() {
284+
return this.loaded.hashCode();
285+
}
286+
287+
}
288+
188289
}

core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,6 @@ public SecurityContext getContext() {
4444
return contextHolder;
4545
}
4646

47-
@Override
48-
public SecurityContext peekContext() {
49-
return contextHolder;
50-
}
51-
5247
@Override
5348
public void setContext(SecurityContext context) {
5449
Assert.notNull(context, "Only non-null SecurityContext instances are permitted");

core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,6 @@ public SecurityContext getContext() {
4444
return ctx;
4545
}
4646

47-
@Override
48-
public SecurityContext peekContext() {
49-
return contextHolder.get();
50-
}
51-
5247
@Override
5348
public void setContext(SecurityContext context) {
5449
Assert.notNull(context, "Only non-null SecurityContext instances are permitted");

core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,6 @@ public static SecurityContext getContext() {
123123
return strategy.getContext();
124124
}
125125

126-
/**
127-
* Peeks the current <code>SecurityContext</code>.
128-
* @return the security context (may be <code>null</code>)
129-
*/
130-
public static SecurityContext peekContext() {
131-
return strategy.peekContext();
132-
}
133-
134126
/**
135127
* Primarily for troubleshooting purposes, this method shows how many times the class
136128
* has re-initialized its <code>SecurityContextHolderStrategy</code>.

core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,6 @@ public interface SecurityContextHolderStrategy {
3838
*/
3939
SecurityContext getContext();
4040

41-
/**
42-
* Peeks the current context without creating an empty context.
43-
* @return a context (may be <code>null</code>)
44-
*/
45-
SecurityContext peekContext();
46-
4741
/**
4842
* Sets the current context.
4943
* @param context to the new argument (should never be <code>null</code>, although

core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,6 @@ public SecurityContext getContext() {
4545
return ctx;
4646
}
4747

48-
@Override
49-
public SecurityContext peekContext() {
50-
return contextHolder.get();
51-
}
52-
5348
@Override
5449
public void setContext(SecurityContext context) {
5550
Assert.notNull(context, "Only non-null SecurityContext instances are permitted");

0 commit comments

Comments
 (0)