Skip to content

Fix: 2625 v3 - IDL only with extra mdsconnect and revert part of #2620#2632

Closed
mwinkel-dev wants to merge 4 commits intoMDSplus:alphafrom
mwinkel-dev:mw-2625-IDL-socket-0-v3
Closed

Fix: 2625 v3 - IDL only with extra mdsconnect and revert part of #2620#2632
mwinkel-dev wants to merge 4 commits intoMDSplus:alphafrom
mwinkel-dev:mw-2625-IDL-socket-0-v3

Conversation

@mwinkel-dev
Copy link
Contributor

Also includes a change to Connections.c to do an integrity check of data structures when adding a connection (aka socket).

@mwinkel-dev mwinkel-dev added US Priority api/idl Relates to the IDL API labels Oct 4, 2023
@mwinkel-dev
Copy link
Contributor Author

Hi @WhoBrokeTheBuild -- Before approving this PR for Fix 3, we should also discuss Fix 4.

@sflanagan
Copy link

Python3 API is still unable to retrieve data in this branch / PR.

See: #2625

@WhoBrokeTheBuild
Copy link
Member

Sean, we're still unable to replicate the python API bug on this branch, but we'll strip out the fix in Connections.c so that this PR is limited to just IDL changes, just in case.

@mwinkel-dev
Copy link
Contributor Author

We have evaluated Fix 3 and Fix 4. Because Fix 3 has a smaller footprint (fewer files changed), we have decided to give priority to Fix 3.

@WhoBrokeTheBuild
Copy link
Member

Update, we're now able to reproduce the python issue, it seems to be related to PR #2620
We're investigating

@mwinkel-dev
Copy link
Contributor Author

We are reverting the change to connection.py that was made in PR #2620.

@mwinkel-dev
Copy link
Contributor Author

Reverting the connection.py change of PR #2620 fixed the Python API bug.

Copy link
Member

Choose a reason for hiding this comment

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

@WhoBrokeTheBuild WhoBrokeTheBuild changed the title Fix: 2625 v3 - IDL only with extra mdsconnect Fix: 2625 v3 - IDL only with extra mdsconnect and revert part of #2620 Oct 4, 2023
@sflanagan
Copy link

I tested the changes again, and a set_database followed by a mdsconnect causes the proxied rdb connection to fail.

See: #2625

@mwinkel-dev
Copy link
Contributor Author

mwinkel-dev commented Oct 5, 2023

Hi @sflanagan -- Please see my post in Issue #2625 (4-Oct-2023 ~8:55 pm EDT). My recollection (perhaps wrong) is that the broken database proxy is caused by a quirk in IDL itself. I will post test results later this evening that illustrate IDL's odd behavior.

Correction: In addition to the IDL quirk, there is indeed an interaction between mdsconnect and set_database that results in a mdsvalue error. That interaction is likely a latent bug that has been in the IDL API for years.

@mwinkel-dev
Copy link
Contributor Author

Added a partial fix for the broken database proxy. It is partial because it works a few times, but eventually fails. See Issue #2625 for details.

@mwinkel-dev
Copy link
Contributor Author

Closing this PR, because the fix is not robust. This PR was never merged, thus no need to revert. For more details, refer to the comments in Issue #2625.

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

Labels

api/idl Relates to the IDL API US Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants