Acqp-323 prevent blank connection errors in STOMP connections

[keithralphs]

Add handler functions to check for Exceptions with message that are None, Empty String or a String that consists only of spaces

Fixes #792

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.45%. Comparing base (07bd6ba) to head (262b642).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
+ Coverage   93.43%   93.45%   +0.01%     
==========================================
  Files          38       38              
  Lines        2103     2108       +5     
==========================================
+ Hits         1965     1970       +5     
  Misses        138      138              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[callumforrester]

Thanks Keith, at the risk of being annoying, please could you reference the Github issue rather than the internal issue? We check those every sprint anyway, so would be good if kept things automatically closing on Github.

Also, please could you make an issue to revisit the EnvironmentResponse API as discussed?

[callumforrester]

Should: Since this just propagates the previous error message it will become WorkerDispatcher: <previous message>, where it was <previous message> before. That feels very inconsistent, the user should see the same message when the same thing breaks. Since this is always set by _safe_exception_message you're probably safe just propagating it without checking?

[keithralphs]

I did wonder whether to do this one. If there is an error message then yes, it would be glueing WorkerDispatcher on the front, which might be inconsistent. It was added for the case where the message was blank, but I think I misread the code at the time, thinking it was an error with stop, which it isn't so should probably remove.

[callumforrester]

Must: Not sure I understand the logic here, BLANK_REPORT being in the error message implies there's been an error without text (i.e. an unknown error), but we should still raise.

[keithralphs]

Yes, this is basically saying if there's an error message, but it's not the one that gets inserted when there's been an exception that didn't have its own message (i.e. it's and error message that actually tells you something) then raise the error. This is equivalent to the previous behaviour, the only difference being that, in that case, no standard message would have been inserted so it would just be None.

[callumforrester]

So from discussion, here is how we want the client to behave:

	initialized=True	initialized=False
error_message is None	Return	Block until next poll
error_message is not None	Undefined	Raise

This is true regardless of whether BLANK_REPORT is present or not, because error_message not being None means an exception has been thrown, expanded table:

	initialized=True	initialized=False
error_message is None	Return	Block until next poll
error_message is not None and BLANK_REPORT in error_message	Undefined	Raise
error_message is not None and BLANK_REPORT not in error_message	Undefined	Raise

[keithralphs]

Issue #806 created to cover implementation and use of this logic

[callumforrester]

LGTM! Thanks