Skip to content

Lock access to facade_factory in data_watchdog to avoid accessing destructed object#5844

Merged
danpat merged 2 commits intomasterfrom
danpat_dataload_segfault_fix
Oct 1, 2020
Merged

Lock access to facade_factory in data_watchdog to avoid accessing destructed object#5844
danpat merged 2 commits intomasterfrom
danpat_dataload_segfault_fix

Conversation

@danpat
Copy link
Copy Markdown
Member

@danpat danpat commented Sep 29, 2020 •
edited
Loading

Issue

We have a race condition in the data_watchdog where a request may still be accessing a facade_factory that has been destructed by the watchdog thread due to new traffic data arriving (loaded by osrm-datastore).

Performance before the lock:

$ wrk -c 8 -t 8 -d 20 "http://localhost:5000/route/v1/car/1,1;2,2?exclude=toll"
Running 20s test @ http://localhost:5000/route/v1/car/1,1;2,2?exclude=toll
  8 threads and 8 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   706.66us  224.89us  10.92ms   94.13%
    Req/Sec     1.42k   193.42     1.68k    90.55%
  227791 requests in 20.10s, 207.90MB read
  Socket errors: connect 0, read 439, write 0, timeout 0
Requests/sec:  11332.57
Transfer/sec:     10.34MB

Performance after the lock:

$ wrk -c 8 -t 8 -d 20 "http://localhost:5000/route/v1/car/1,1;2,2?exclude=toll"
Running 20s test @ http://localhost:5000/route/v1/car/1,1;2,2?exclude=toll
  8 threads and 8 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   761.02us    1.49ms  51.52ms   99.72%
    Req/Sec     1.43k   173.74     1.72k    86.07%
  229274 requests in 20.10s, 209.25MB read
  Socket errors: connect 0, read 443, write 0, timeout 0
Requests/sec:  11405.21
Transfer/sec:     10.41MB

My read on this is that the new shared lock has negligible impact on overall performance.

With the shared lock in place, I'm no longer able to reproduce the crash described in

#5841

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@danpat danpat changed the title Make a copy of the excludable classes so they can still be accessed d… Make a copy of the excludable classes so they can be accessed during data exchange Sep 29, 2020
@danpat danpat mentioned this pull request Sep 29, 2020
Closed
@danpat danpat changed the title Make a copy of the excludable classes so they can be accessed during data exchange Lock access to facade_factory in data_watchdog to avoid accessing destructed object Sep 30, 2020
@danpat
Wrap access to facade_factory in a shared lock so it doesn't get chan…
8638a13 
…ged partway through access which leads to a crash.
@danpat danpat force-pushed the danpat_dataload_segfault_fix branch from ba78fa8 to 8638a13 Compare September 30, 2020 14:45
@danpat danpat marked this pull request as ready for review September 30, 2020 16:50
@danpat danpat merged commit 3451d1e into master Oct 1, 2020
@danpat danpat deleted the danpat_dataload_segfault_fix branch October 1, 2020 01:45
datendelphin added a commit to fossgis-routing-server/osrm-backend that referenced this pull request Nov 19, 2020
@datendelphin
Merge tag 'v5.23.0'
a823897 
  - Changes from 5.22.0
    - Build:
      - FIXED: pessimistic calls to std::move [Project-OSRM#5560](Project-OSRM#5561)
    - Features:
      - ADDED: new API parameter - `snapping=any|default` to allow snapping to previously unsnappable edges [Project-OSRM#5361](Project-OSRM#5361)
      - ADDED: keepalive support to the osrm-routed HTTP server [Project-OSRM#5518](Project-OSRM#5518)
      - ADDED: flatbuffers output format support [Project-OSRM#5513](Project-OSRM#5513)
      - ADDED: Global 'skip_waypoints' option [Project-OSRM#5556](Project-OSRM#5556)
      - FIXED: Install the libosrm_guidance library correctly [Project-OSRM#5604](Project-OSRM#5604)
      - FIXED: Http Handler can now deal witch optional whitespace between header-key and -value [Project-OSRM#5606](Project-OSRM#5606)
    - Routing:
      - CHANGED: allow routing past `barrier=arch` [Project-OSRM#5352](Project-OSRM#5352)
      - CHANGED: default car weight was reduced to 2000 kg. [Project-OSRM#5371](Project-OSRM#5371)
      - CHANGED: default car height was reduced to 2 meters. [Project-OSRM#5389](Project-OSRM#5389)
      - FIXED: treat `bicycle=use_sidepath` as no access on the tagged way. [Project-OSRM#5622](Project-OSRM#5622)
      - FIXED: fix table result when source and destination on same one-way segment. [Project-OSRM#5828](Project-OSRM#5828)
      - FIXED: fix occasional segfault when swapping data with osrm-datastore and using `exclude=` [Project-OSRM#5844](Project-OSRM#5844)
      - FIXED: fix crash in MLD alternative search if source or target are invalid [Project-OSRM#5851](Project-OSRM#5851)
    - Misc:
      - CHANGED: Reduce memory usage for raster source handling. [Project-OSRM#5572](Project-OSRM#5572)
      - CHANGED: Add cmake option `ENABLE_DEBUG_LOGGING` to control whether output debug logging. [Project-OSRM#3427](Project-OSRM#3427)
      - CHANGED: updated extent of Hong Kong as left hand drive country. [Project-OSRM#5535](Project-OSRM#5535)
      - FIXED: corrected error message when failing to snap input coordinates [Project-OSRM#5846](Project-OSRM#5846)
    - Infrastructure
      - REMOVED: STXXL support removed as STXXL became abandonware. [Project-OSRM#5760](Project-OSRM#5760)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akashihi akashihi akashihi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@danpat @akashihi