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.

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

Ensure duplexer stop entails a websocket close #3246

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

jondequinor
Copy link
Contributor

@jondequinor jondequinor commented Apr 9, 2022

Issue
Resolves #3137

If immediately stopped, the duplexer might not have closed the
connection it made. This will appear as a fault on the server side
with a code 1006 (connection closed abnormally), and the server handler
will terminate expectedly.

This change will ensure that a duplexer cannot be used until a
connection has been made, meaning stop will close the connection and not fail.

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@jondequinor jondequinor self-assigned this Apr 9, 2022
@jondequinor jondequinor marked this pull request as draft April 9, 2022 07:51
@jondequinor jondequinor added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Apr 10, 2022
@jondequinor jondequinor marked this pull request as ready for review April 10, 2022 17:34
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2022

Codecov Report

Merging #3246 (52965c8) into main (de18099) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3246   +/-   ##
=======================================
  Coverage   65.61%   65.62%           
=======================================
  Files         619      619           
  Lines       48887    48894    +7     
  Branches     4404     4404           
=======================================
+ Hits        32076    32085    +9     
+ Misses      15310    15307    -3     
- Partials     1501     1502    +1     
Impacted Files Coverage Δ
ert_shared/ensemble_evaluator/evaluator.py 95.95% <100.00%> (-0.03%) ⬇️
ert_shared/ensemble_evaluator/sync_ws_duplexer.py 90.00% <100.00%> (+1.11%) ⬆️
libres/lib/enkf/block_fs_driver.cpp 80.23% <0.00%> (-0.59%) ⬇️
ert/data/record/_transformation.py 88.57% <0.00%> (-0.48%) ⬇️
libres/lib/res_util/block_fs.cpp 51.62% <0.00%> (ø)
ert_gui/simulation/run_dialog.py 81.60% <0.00%> (+0.76%) ⬆️
ert_shared/ensemble_evaluator/monitor.py 93.33% <0.00%> (+3.33%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@verveerpj verveerpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one tiny question.

Copy link
Contributor

@BjarneHerland BjarneHerland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

- If immediately stopped, the duplexer might not have closed the
connection it made. This will appear as a fault on the server side
with a code 1006 (connection closed abnormally), and the server handler
will terminate expectedly. This change will ensure that a duplexer
cannot be used until a connection has been made, meaning `stop` will
have closed the connection.

- This fix also revealed a programming error in the prefect ensemble
where a failure in running the flow would not be handled properly, such
as in the case of cancellation. See also
equinor#2759 which discusses failures.

- Also renamed tests so that they will not be confused with each other.
@jondequinor jondequinor enabled auto-merge (rebase) April 12, 2022 08:28
@jondequinor jondequinor merged commit a8a3d32 into equinor:main Apr 12, 2022
@jondequinor jondequinor deleted the fix-3137.1 branch April 12, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing test_ensemble_evaluator_run_and_get_successful_realizations_connection_closed_no_recover
4 participants