Hacky but workable fix for race condition surfaced by If-None-Exists creates #660

Merged
merged 1 commit into from Jun 8, 2017

Conversation

Projects
None yet
3 participants
@karlmdavis
Contributor

karlmdavis commented Jun 1, 2017

Per our discussion here: 3db4091

Some notes from other attempts I made to fix this in a less hacky way:

  • Tried to @Autowire myResourceDaos from a setter (rather than using the annotation on a field), and initialize myResourceTypeToDao in that setter, instead. Couldn't get it to work: Spring started throwing odd bean dependency errors. Don't really understand why.
  • Tried to move the exceptions being thrown on null getDao(Class) results into that method, but that was breaking a test case. Didn't investigate why.
Hacky but workable fix for race condition surfaced by If-None-Exists …
…creates.

Some notes from other attempts I made to fix this in a less hacky way:

* Tried to @autowire myResourceDaos from a setter (rather than using the annotation on a field), and initialize myResourceTypeToDao in that setter, instead. Couldn't get it to work: Spring started throwing odd bean dependency errors. Don't really understand why.
* Tried to move the exceptions being thrown on null getDao(Class) results into that method, but that was breaking a test case. Didn't investigate why.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 1, 2017

Coverage Status

Coverage decreased (-0.007%) to 76.723% when pulling aa134fc on HHSIDEAlab:fix-race-condition-in-if-none-exists into 52a5fcc on jamesagnew:master.

Coverage Status

Coverage decreased (-0.007%) to 76.723% when pulling aa134fc on HHSIDEAlab:fix-race-condition-in-if-none-exists into 52a5fcc on jamesagnew:master.

@karlmdavis

This comment has been minimized.

Show comment
Hide comment
@karlmdavis

karlmdavis Jun 1, 2017

Contributor

Confirmed fix: with this in place, I've got an in-progress benchmark run that has made it through over 3M Bundles, whereas before this change it was dying after a few thousand.

@jamesagnew: I don't think the code coverage difference here is meaningful, but it's your project, not mine. In general, a regression test case for this issue would need to push... 30K or so resources into a FHIR server asynchronously, using... at least dozens of threads. (Like I said, our benchmarks were dying after a few thousand requests, but they were using 390 threads on giant EC2 instances. We just weren't seeing this in our local dev builds/runs.) Thoughts?

Contributor

karlmdavis commented Jun 1, 2017

Confirmed fix: with this in place, I've got an in-progress benchmark run that has made it through over 3M Bundles, whereas before this change it was dying after a few thousand.

@jamesagnew: I don't think the code coverage difference here is meaningful, but it's your project, not mine. In general, a regression test case for this issue would need to push... 30K or so resources into a FHIR server asynchronously, using... at least dozens of threads. (Like I said, our benchmarks were dying after a few thousand requests, but they were using 390 threads on giant EC2 instances. We just weren't seeing this in our local dev builds/runs.) Thoughts?

karlmdavis added a commit to CMSgov/hapi-fhir that referenced this pull request Jun 2, 2017

karlmdavis added a commit to CMSgov/bluebutton-data-server that referenced this pull request Jun 2, 2017

Bumped to HAPI 2.4.
Actually alightly-hacked local/fake release of it that incorporates jamesagnew/hapi-fhir#660.

CBBD-262

@karlmdavis karlmdavis referenced this pull request in CMSgov/bluebutton-data-server Jun 2, 2017

Merged

CBBD-262: Bumped to HAPI 2.4 #30

karlmdavis added a commit to CMSgov/bluebutton-data-pipeline that referenced this pull request Jun 2, 2017

Bumped to latest HAPI 2.4 release.
This local/fake release incorporates jamesagnew/hapi-fhir#660, which we need.

CBBD-262
CBBI-211

@karlmdavis karlmdavis referenced this pull request in CMSgov/bluebutton-data-pipeline Jun 2, 2017

Merged

CBBI-211: FHIR auto-retry and idempotency #106

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Jun 8, 2017

Owner

This doesn't even seem hacky to me. :)

Thanks for the investigation and fix! Merging it now.

Owner

jamesagnew commented Jun 8, 2017

This doesn't even seem hacky to me. :)

Thanks for the investigation and fix! Merging it now.

jamesagnew added a commit that referenced this pull request Jun 8, 2017

@jamesagnew jamesagnew merged commit b6e9a75 into jamesagnew:master Jun 8, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.007%) to 76.723%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@karlmdavis

This comment has been minimized.

Show comment
Hide comment
@karlmdavis

karlmdavis Jun 8, 2017

Contributor

It just feels wrong to let it enter that block and redo that work if initialized by multiple threads at once. But I couldn't get Spring to play nice, so oh well.

Contributor

karlmdavis commented Jun 8, 2017

It just feels wrong to let it enter that block and redo that work if initialized by multiple threads at once. But I couldn't get Spring to play nice, so oh well.

@karlmdavis karlmdavis deleted the CMSgov:fix-race-condition-in-if-none-exists branch Oct 3, 2017

@karlmdavis karlmdavis restored the CMSgov:fix-race-condition-in-if-none-exists branch Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment