Fix: improve mdstcl's error handling and add comments #2746
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 23, 2024
added 6 commits
April 24, 2024 13:59
mwinkel-dev
force-pushed
the
mw-2731-actions-v4
branch
from
fe0f045
to
6453f20
Compare
WhoBrokeTheBuild
approved these changes
Apr 25, 2024
| @@ -82,7 +82,7 @@ static int do_login(Connection *c) | |||
| int status = SendMdsMsgC(c, msend, 0); | |||
| int err; | |||
| free(msend); | |||
| if (STATUS_NOT_OK) | |||
| if ((status == SsINTERNAL) || STATUS_NOT_OK) | |||
There was a problem hiding this comment.
In the other changes, we rewrite status to MDSplusERROR, but in this one we just check for SsINTERNAL as well, is there a reason for the difference?
There was a problem hiding this comment.
No, I don't have a good reason. Other than the changes were made over several days, thus I was inconsistent.
The longer answer . . .
I'm taking a cautious approach until we all reach agreement on the broader SsINTERNAL problem, Issue #2741.
Timo tells me it is important to keep SsINTERNAL so that it ripples up the call stack. However, most of the code I've looked at never checks for SsINTERNAL. This is a systemic error throughout all of MDSplus. This PR is just a targeted fix for the issues I saw while investigating the dispatcher's leaking sockets.
My preference would be to redefine SsINTERNAL so it is a valid MDSplus error code (i.e., set it to -2 so that the low order bit is zero, meaning failure). Timo and I have not reached agreement on whether that is feasible or not.
WhoBrokeTheBuild
pushed a commit
that referenced
this pull request
May 21, 2024
* add comments regarding action service * send_reply() now does cleanup_client() on bad socket * explain mdstcl's receiver thread cannot access main thread's connection list * Improve handling of non-MDSplus error codes * add comments regarding action dispatch * add comment explaining receiver thread select loop
WhoBrokeTheBuild
added a commit
that referenced
this pull request
May 21, 2024
* Gm apd java (#2729) * Improve APD support for Java interface * Improve APD support for Java - forgotten files * Commit packages * When activate debug trace, now compiles without error. (#2735) This fixes Issue 2734. * Fix: reduce open files due to dispatcher (#2740) * Fix: reuse action_server connection id in ServerConnect; avoid duplicates in list * Fix: set dispatched early; unset if dispatching failed; prevent race on fast actions * Fix: lock Clients in ServerQAction; cleanup and check before use * Fix: reconnect dropped connections * Fix: use correct windows SOCKET print format * Fix: satisfy rhel7 c standard * Gm apd thin cpp (#2742) * Added ADP support in C++ thin client * Added tdi fun * Added TDI FUn * Fix commands * Gm new marte (#2743) * more parameters for marte2_simulink_generic * Proceed with the new implementation * Proceed * Proceed * Proceed * Proceed * Proceed * proceed * Proceed * Proceed * Partially tested version * Added execution times recording * Proceed * Procced with debugging * Proceed * Proceed * Proceed * Fixes for multisampled acquisition * Remove quotes from string parameters * Minor fixes * Procced debugging * Debugging * More channels * Debug Distributed configuration * Fix sognal recording for synchronized inputs * Further debug * Further debug * Small fixes * Close ti final version * Forgotten fix * Make port visible, fix parameter name * unaligned nids * Increase DiscontinuityFactor * Discontinuityfactor * More channels * Proceed with the new implementation * Proceed * Proceed * Proceed * Proceed * Proceed * proceed * Proceed * Proceed * Partially tested version * Added execution times recording * Proceed * Procced with debugging * Proceed * Proceed * Proceed * Fixes for multisampled acquisition * Remove quotes from string parameters * Minor fixes * Procced debugging * Debugging * More channels * Debug Distributed configuration * Fix sognal recording for synchronized inputs * Further debug * Further debug * Small fixes * Close ti final version * Forgotten fix * Make port visible, fix parameter name * unaligned nids * Increase DiscontinuityFactor * Discontinuityfactor * More channels * Packages updated * Remove print * Remove error messages --------- Co-authored-by: mdsplus <mdsplus@roactive2.rfx.local> * Docs: Improve documentation for getSegment* python wrappers (#2732) Add explanation and rename parameters for: * getSegmentLimits * getSegmentList * Fix: Update JAVASOURCE to 8 to support JDK 17 (#2747) * Fix: improve mdstcl's error handling and add comments (#2746) * add comments regarding action service * send_reply() now does cleanup_client() on bad socket * explain mdstcl's receiver thread cannot access main thread's connection list * Improve handling of non-MDSplus error codes * add comments regarding action dispatch * add comment explaining receiver thread select loop * Fix: multiple string escape warnings thrown by python 12 (#2748) ``` mdsplus/pydevices/RfxDevices/FAKECAMERA.py:40: SyntaxWarning: invalid escape sequence '\C' {'path': ':EXP_NODE', 'type': 'text', 'value': '\CAMERATEST::FLIR:FRAMES'}, mdsplus/pydevices/RfxDevices/PLFE.py:220: SyntaxWarning: invalid escape sequence '\#' '^(\#[0-5][01]([01][0-9][0-9]|2[0-4][0-9]|25[0-5])){6}$', msg) mdsplus/pydevices/RfxDevices/CYGNET4K.py:361: SyntaxWarning: invalid escape sequence '\E' self.serialIO(b'\x55\x99\x66\x11\x50\EB', None) mdsplus/pydevices/RfxDevices/CYGNET4K.py:461: SyntaxWarning: invalid escape sequence '\8' return self.setValue(b'\81\x82', min(0xFFF, value), True) mdsplus/pydevices/MitDevices/dt100.py:161: SyntaxWarning: invalid escape sequence '\.' regstr = '([0-9\.]*) [0-9] ST_(.*)\r\n' ``` The \CAMERATEST became \\CAMERATEST The regex strings should be python r-strings `r""`, but to maintain backwards compatibility, we're using \\ The broken hex-codes now have x in them * Build: Resolve linker error after updating the windows builder to Fedora 39 (#2749) * Build: Resolve linker error after updating the windows builder to Fedora 39 This appeared after updating the mdsplus/builder:windows docker image to Fedora 39, and Wine to 9.0 The newer libxml2 tried to link dynamically unless we explicitly set LIBXML_STATIC * Hopefully fix the MdsTreeNodeTest It turns out that this was failing previously, but we weren't properly catching the error * Fix errors in windows build from newer gcc * Docs: Update sites.csv (#2615) add Startorus Fusion in Xi'an, China * Fix: mdsip now sends proper auth status back to the client (#2752) Fixes issues #2750 and #2652 * Fix: mdstcl's `show current` no longer segfaults when no tree paths defined (#2754) * Fix: "show current" no longer segfaults when no tree paths defined * Fix: corrected typo in error message * Use original error message so tests pass * Fix: Add Debian 12 and Ubuntu 24.04 and support GCC 12+ (#2753) * Build: Add Debian 12 and Ubuntu 24.04 * Add extra flags for GCC 12+ and stub imp for Python 3.12 GCC 12+ triggers a bunch of false positive warnings (which we treat as errors) This adds AX_C_FLAGS to configure those `-Wno-*` flags for GCC 12+ `cmdExecute.c` now uses snprintf to avoid buffer overflow warnings, also generated by GCC 12+ `compound.py.in` now supports Python 3.12+ * compound.py now supports Python 2.7.. again --------- Co-authored-by: Stephen Lane-Walsh <slwalsh@psfc.mit.edu> * Fix: Improve error messaging when calling Setup Device in jTraverser (#2744) * Improve error messaging when calling Setup Device in jTraverser e.getMessage() sometimes returned null, but just e will always print something Add a printStackTrace() for InvocationTargetException exceptions to show the encapsulated error * Add import for InvocationTargetException * Build: Fix off-by-one versions produced by Jenkins (#2756) This fixes the bug where `--os=bootstrap` wasn't receiving the version from `--version=x.y.z` However, confusingly, this also changes the Jenkinsfile to not use that feature, and instead use `git tag` in order to embed the proper git information as well as the proper version information The `--os=bootstrap` and `--version` fix is still included just so that it doesn't break if someone else tries to use it * Build: Increase default test timeout to 1h (#2757) When the build server(s) are at capacity, it's not unreasonable for a test to take more than 10 seconds, which was the old default timeout This sets the default to 1h, and removes the overrides in various tests * Gm fix filter (#2755) * Allow filtering data from MinMax resampling; remove useless thread in jServer * Fix compile error * Remove debug message * Make Windows Compiler happy * Build: Fix 'HEAD' in `show version` and tag error (#2758) Jenkins builds in a detached HEAD state, which caused bootstrap to use HEAD as the branch name We pass --branch= to the bootstrap call in Jenkins, but $BRANCH wasn't being passed into the bootstrap docker container Also, attempts to build alpha versions with tags that already existed failed * Fix: mdstcl show version tag and links (#2760) Fixes Issue #2759 * Feature: CompileTree will exit with non-zero status code for error messages. (#2446) And error message should go to stderr. * Build: Add package override for ubuntu and debian (#2761) Override sections for Ubuntu 24 and Debian Bookworm were added. * Fix: Python release version tag (#2764) * Feature: Add "Date:" to show version output (#2767) Implements #2766 Example: ``` $ mdstcl sho ver MDSplus version: 7.140.75 ---------------------- Release: alpha_release-7-140-75 Date: Thu May 16 17:43:14 UTC 2024 Browse: https://github.com/MDSplus/mdsplus/tree/alpha_release-7-140-75 Download: https://github.com/MDSplus/mdsplus/releases/tag/alpha_release-7-140-75 ``` * Fix: remove abort flag from RfxDevices DIO2 initialization (#2769) Fixes issue #2768 * Fix: Missing repo metadata signing (#2770) This will hopefully fix the lack of signed metadata files that are preventing us from automatically publishing releases --------- Co-authored-by: GabrieleManduchi <gabriele.manduchi@igi.cnr.it> Co-authored-by: mwinkel-dev <122583770+mwinkel-dev@users.noreply.github.com> Co-authored-by: Timo Schroeder <zack-vii@users.noreply.github.com> Co-authored-by: mdsplus <mdsplus@roactive2.rfx.local> Co-authored-by: Josh Stillerman <jas@psfc.mit.edu> Co-authored-by: Fernando Santoro <44955673+santorofer@users.noreply.github.com> Co-authored-by: Louwrensth <Louwrensth@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes some problems found during the investigation of Issue #2731.