Skip to content

Fix: remove extra call serializeout connection.py#2716

Closed
joshStillerman wants to merge 2 commits intoalphafrom
jas-fix-connection-py
Closed

Fix: remove extra call serializeout connection.py#2716
joshStillerman wants to merge 2 commits intoalphafrom
jas-fix-connection-py

Conversation

@joshStillerman
Copy link
Contributor

#2661 changed the expr to be evaluated enclosing it in a call to serializeout

When connecting to older mdsip servers - like:

TCL> show version

MDSplus version: 7.78.6
----------------------
  Release:  alpha_release-7-78-6
  Browse:   https://github.com/MDSplus/mdsplus/tree/alpha_release-7-78-6
  Download: https://github.com/MDSplus/mdsplus/archive/alpha_release-7-78-6.tar.gz
  Build date: Wed Jun 26 18:51:44 UTC 2019

TCL>

This causes calls like TreeOpen('tree', shot) to generate an infinte recursion error,

This PR removes line 208 from connection.py

GABRIELE: Does this break #2660/#2661 ??

#2661 changed the expr
to be evaluated enclosing it in a call to `serializeout`

When connecting to older mdsip servers - like:
```
TCL> show version

MDSplus version: 7.78.6
----------------------
  Release:  alpha_release-7-78-6
  Browse:   https://github.com/MDSplus/mdsplus/tree/alpha_release-7-78-6
  Download: https://github.com/MDSplus/mdsplus/archive/alpha_release-7-78-6.tar.gz
  Build date: Wed Jun 26 18:51:44 UTC 2019

TCL>
```
This causes calls like TreeOpen('tree', shot) to generate an infinte
recursion error,

This PR removes line 208 from connection.py

GABRIELE:  Does this break #2660/#2661 ??
@joshStillerman joshStillerman added the bug An unexpected problem or unintended behavior label Feb 20, 2024
@mwinkel-dev
Copy link
Contributor

mwinkel-dev commented Feb 20, 2024

A related APD change is in the earlier PR #2620.

@joshStillerman
Copy link
Contributor Author

Gabriele -
It could be that:

  • The code should check if it is an APD instead of just 'not serializing' as I did in this PR
  • Whatever the fix turns out to be, it probably will be needed in all of the languages that you fixed

@joshStillerman
Copy link
Contributor Author

Gabriele -
It could be that:

  • The code should check if it is an APD instead of just 'not serializing' as I did in this PR
  • Whatever the fix turns out to be, it probably will be needed in all of the languages that you fixed

@mwinkel-dev
Copy link
Contributor

mwinkel-dev commented Feb 20, 2024

Hi @joshStillerman,

This fix was manually tested with MATLAB R2023b on Ubuntu 20 and it successfully read and plotted a signal (i.e., Issue #2660 did not reappear). The signal was retrieved without error regardless of whether connecting to server A or server B.

However, a cross-version error was found as is shown below.

Success

>> 
>> mdsconnect('<server A>');           % has MDSplus alpha_release_7.112.1
>> mdsopen('cmod',1090909009);
>> mdsclose();
>> mdsdisconnect();
>> 

Failure

>> 
>> mdsconnect('<server B>');           % has MDSplus alpha_release-7-78-6
>> mdsopen('cmod',1090909009);
>> mdsclose();

ans =

    'Java exception occurred: 
     MDSplus.MdsException: %TDI-E-RECURSIVE, Overly recursive function, calls itself maybe
     	at MDSplus.Connection.get(Native Method)
     	at MDSplus.Connection.get(Connection.java:117)'

>> mdsdisconnect();
>>

@joshStillerman
Copy link
Contributor Author

Do not merge - needs discussion

@mwinkel-dev
Copy link
Contributor

See comments in PR #2720.

@mwinkel-dev
Copy link
Contributor

mwinkel-dev commented Feb 27, 2024

Initial bug report was for thin-client with the Python API's connection.openTree(<tree>, <shot>) method. And the alpha-7-78-6 was probably running on an Ubuntu 18 server.

@mwinkel-dev
Copy link
Contributor

Closing this PR as per @joshStillerman. Refer to PR #2720 for the full history of the APD related problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug An unexpected problem or unintended behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants