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.

Sign up for GitHub

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

Jump to bottom

RefreshablePeerEurekaNodesTests has 2 broken tests in master #2538

Closed
spencergibb opened this issue Dec 12, 2017 · 9 comments
Closed

RefreshablePeerEurekaNodesTests has 2 broken tests in master #2538

spencergibb opened this issue Dec 12, 2017 · 9 comments
Milestone
2.0.0.RELEASE

Comments

@spencergibb
Copy link
Member

updatedWhenAvailabilityZoneChanged() and updatedWhenRegionChanged()
@spencergibb spencergibb added this to the 2.0.0.M6 milestone Dec 12, 2017
@spencergibb
Copy link
Member Author

Tests commented out in 170739e
/cc @fahimfarookme @ryanjbaxter

@ryanjbaxter
Copy link
Contributor

Thanks. I started to look at this and then had to get on my flight back from Spring One. You would have thought that the build on CircleCI would have caught this. I went back and looked at the PR and realized that the build was never run....weird. #2455

@fahimfarookme
Copy link
Contributor

fahimfarookme commented Dec 12, 2017 •
edited
Loading

The build on CircleCI passed in the PR #2455.

Running org.springframework.cloud.netflix.eureka.server.RefreshablePeerEurekaNodesTests
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.252 sec

I'm looking into the issue on master.

@fahimfarookme
Copy link
Contributor

fahimfarookme commented Dec 14, 2017 •
edited
Loading

PFB the analysis so far on failing test cases.

  • This test cases are passing on my PR branch - which is based on spring-boot 1x / spring-cloud-commons 1x.
  • Once merged it's failing only on master - which is based on spring-boot 2x / spring-cloud-commons 2x.
  • On master, EurekaClientConfigBean#availabilityZones is not being updated except for very first EnvironmentChangeEvent hence the above test case is failing.
  • This is in fact a generic issue - that is; In spring-boot 2x / spring-cloud-commons 2x,
    Maps in @ConfigurationProperties are not being updated from the second EnvironmentChangeEvent onwards.
  • I did a PoC here to confirm that - the same set of test cases passed here (spring-boot 1x), but failed here (spring-boot 2x).

i.e. Following test fails in spring-boot 2x, passed in spring-boot 1x

@ConfigurationProperties
class Props {
   private String str;
   private Map<String, String> map = new HashMap<>();
   ...
}

// first EnvironmentChangeEvent
TestPropertyValues.of("str=str-value").applyTo(this.environment);
context.publishEvent(new EnvironmentChangeEvent(..."str"));
// 'str-value' is reflected in @ConfigurationProperties bean - in both spring-boot 1x and 2x

// second EnvironmentChangeEvent - updates the map
TestPropertyValues.of("map.key=map-value").applyTo(this.environment);
context.publishEvent(new EnvironmentChangeEvent(..."map.key"));
// 'map-value' is NOT reflected in @ConfigurationProperties bean - in spring-boot 2x

// fails only in spring-boot 2x	
assertEquals("map-value", context.getBean(Props.class).getMap().get("key"));

Can this be a side effect from spring-cloud-context' upgrade to spring-boot-2 and part of this #10685? Maybe @spencergibb @dsyer can shed some lights?

I will anyway investigate it further tonight.

@ryanjbaxter ryanjbaxter modified the milestones: 2.0.0.M6, 2.0.0.RELEASE May 2, 2018
@ryanjbaxter ryanjbaxter modified the milestones: 2.0.0.RC2, 2.0.0.RC3 May 24, 2018
@spencergibb spencergibb modified the milestones: 2.0.0.RC3, 2.0.0.RELEASE May 31, 2018
fahimfarookme added a commit to fahimfarookme/spring-boot-pr that referenced this issue Jun 3, 2018
@fahimfarookme
Maps in @ConfigurationProperties are not being updated from the second
8d1d69d 
EnvironmentChangeEvent onwards

## Issue
In spring-boot 2x `Maps` in `@ConfigurationProperties` are not being
updated from the **second** `EnvironmentChangeEvent` onwards. i.e.
Following test fails in spring-boot 2x, but passed in spring-boot 1x.

```
@ConfigurationProperties
class Props {
   private String str;
   private Map<String, String> map = new HashMap<>();
   ...
}

// first EnvironmentChangeEvent
// 'str-value' gets reflected in @ConfigurationProperties bean - in both
spring-boot 1x and 2x
TestPropertyValues.of("str=str-value").applyTo(this.environment);
context.publishEvent(new EnvironmentChangeEvent(..."str"));

// second EnvironmentChangeEvent - updates the map
// 'map-value' NOt getting reflected in @ConfigurationProperties bean -
in spring-boot 2x
TestPropertyValues.of("map.key=map-value").applyTo(this.environment);
context.publishEvent(new EnvironmentChangeEvent(..."map.key"));

// fails only in spring-boot 2x
assertEquals("map-value",
context.getBean(Props.class).getMap().get("key"));
```

## Analysis
* The `MapBinder` [iterates
over](https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java#L152)
`SpringIterableConfigurationPropertySource` in order to bind properties
to the map.
* However `SpringIterableConfigurationPropertySource#iterator()` having
reference only to the initial/ old property-keys. This is because of the
following bug in `Cache` implementation;
   - In case of `MapPropertySource`,
`SpringIterableConfigurationPropertySource#getCacheKey()`
[here](https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java#L146)
always returns the same object in the heap (for different invocations),
even though the object (i.e. key-set) state has changed. i.e. <br/>
   `return ((MapPropertySource)
getPropertySource()).getSource().keySet();`

   - As a result, `if (ObjectUtils.nullSafeEquals(cacheKey,
this.cacheKey))` [here](https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java#L136)
always evaluates to true and returns the old/ initial cache.

## Fix
`SpringIterableConfigurationPropertySource#getCacheKey()` should return
a new object. This branch - `if (getPropertySource() instanceof
MapPropertySource)` is not necessary.

```
private Object getCacheKey() {
   return getPropertySource().getPropertyNames(); // returns a new array
}
```
Whenever the `cacheKey` is updated, `Cache` is reinitialized (with all
the property keys)

## Sample
* PoC [here](https://github.com/fahimfarookme/configuration-properties-maps-issue/tree/master)
to confirm/ recreate the issue - the same set of test cases passed
[here](https://github.com/fahimfarookme/configuration-properties-maps-issue/tree/master/spring-boot-1x-cloud-1x)
(spring-boot 1x), but failed
[here](https://github.com/fahimfarookme/configuration-properties-maps-issue/tree/master/spring-boot-2x-cloud-2x)
(spring-boot 2x).


This issue supersedes [this
one](spring-cloud/spring-cloud-netflix#2538).
@fahimfarookme fahimfarookme mentioned this issue Jun 3, 2018
Closed
@fahimfarookme
Copy link
Contributor

fahimfarookme commented Jun 3, 2018 •
edited
Loading

Sorry for the delayed response.
Failing test cases above are due to a spring-boot-2 bug. PR raised here #13344. Should be able to uncomment the test cases if #13344 is merged.

@ryanjbaxter
Copy link
Contributor

Great thanks for looking at this, we will monitor the PR in Boot.

@philwebb
Copy link
Contributor

philwebb commented Jun 6, 2018 •
edited
Loading

spring-projects/spring-boot#13344 is fixed. I removed the @Ignores from RefreshablePeerEurekaNodesTests locally and they seem OK now.

@ryanjbaxter
Copy link
Contributor

still not passing for me...maybe the snapshots havent been published yet, i will try again later

@fahimfarookme
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.0.RELEASE
Development

No branches or pull requests
4 participants
@philwebb @ryanjbaxter @spencergibb @fahimfarookme