Let client ping failure clean up session - #13655
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-3.4 #13655 +/- ##
===========================================
Coverage 92.57% 92.58%
===========================================
Files 323 323
Lines 20525 20529 +4
===========================================
+ Hits 19002 19007 +5
+ Misses 1523 1522 -1 |
Could this just be a race condition, i.e. sometimes the failed ping is just what happens to show up first sometimes when a connection drops? Regardless, I agree that a failed ping should not explode the world. |
|
FWIW I think |
|
When we see this this happens continuously until the server is restarted causing other sessions to drop early because they're not receiving the pings so it doesn't just seem like a race condition. I'll add a test tomorrow so let's wait for that to merge. |
|
Note that this is pretty rare but is pretty bad when it does occur. In a production app that serves about 10-300 daily user sessions this happens roughly once every 1-2 months. |
|
Do we want to backport this for a 3.3.4? That would also let us fix a current docs issue without a manual re-deploy |
I certainly would love that and it would make our client happy. |
|
@philippjfr looks like just some codebase issues. If you can get this merged today I can start the backport PR tonight. |
|
The pandas issues are still causing failures. I've got to log off now, so if you're still around @mattpap could you figure out what to do about that? |
b4bccfa to
7cd1244
Compare
|
I suspect the test failures are spurious. I am going to go ahead and merge this so that I can get the backport branch started (assent to merge seemed to be implied above) |
* Let client ping failure clean up session * Fix typo * Add test * Fix codestyle issues
* Log figure render count and start/end (#13503) * Log PlotView render count * Include model id in render console messages * remove new code annotations * test logger trace * implement log level trace handling * remove extra semicolons apply suggested message * apply suggested message --------- Co-authored-by: Ian Thomas <ianthomas23@gmail.com> * Update CoC to use reporting form (#13647) * Let client ping failure clean up session (#13655) * Let client ping failure clean up session * Fix typo * Add test * Fix codestyle issues * Ignore pandas' deprecation warning regarding pyarrow (#13657) * Ignore pandas' deprecation warning regarding pyarrow * Fix other deprecation warnings * Support pandas 1.x and 2.x/3.x simultaneously * Fix missing User Guide sidebars (#13659) * Update environment-release-build.yml * Use pip syntax * Force import line wrap by count and line length (#13626) * Make line_length cutoff consistent at 88 * Update the codebase * Use relative imports to shorten line lengths * add rst-files for glyphes (#13597) * Update app.rst (#13588) * Add Metadata for airports_graph.py and geojson_source.py (#13568) * Added metadata to airports_graph.py * Added metadata to geojson_source.py * Remove trailing whitespaces * Fix formating of metadata, remove some apis from airports_graph.py * Add space between tripple qoute and first letter * remove trailing white space * Add Iterable kind and use it in BooleanFilter.booleans (#13661) * update switcher.json and release notes * baseline test manual fixup --------- Co-authored-by: Demetris Roumis <roumis.d@gmail.com> Co-authored-by: Ian Thomas <ianthomas23@gmail.com> Co-authored-by: Pavithra Eswaramoorthy <pavithraes@outlook.com> Co-authored-by: Philipp Rudiger <prudiger@anaconda.com> Co-authored-by: Mateusz Paprocki <mattpap@gmail.com> Co-authored-by: Moritz Schreiber <68053396+mosc9575@users.noreply.github.com> Co-authored-by: Icoti <94801369+Icoti@users.noreply.github.com> Co-authored-by: dasha-moskv <81783162+dasha-moskv@users.noreply.github.com>
* Let client ping failure clean up session * Fix typo * Add test * Fix codestyle issues
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
Ideally we would figure out why server clients are sometimes not cleaned up correctly, however as a safeguard I think it is still a good idea to add error handling around the keep alive pings so that if a single client isn't cleaned up correctly we don't end up breaking pings for all other clients.