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

Data flow persists even after closing the downlink #156

Closed
cyanaryan opened this issue Jan 17, 2024 · 25 comments
Closed

Data flow persists even after closing the downlink #156

cyanaryan opened this issue Jan 17, 2024 · 25 comments
Assignees
Labels
C-question Category: question

Comments

@cyanaryan
Copy link

I wanted to know about how to close the downlink so that it gets terminated and the data flow stops from that downlink.
Like for example I am using one downlink. ( I am using javascript)

private demoDownlink = ValueDownlink<ValueDownlink<Value, AnyValue>

this.demoDownlink = host
        .downlinkValue()
        .nodeUri('demo_node_uri')
        .laneUri('demo_lane_uri')
        .didSet((value) => {
          console.log(value.stringValue());      
        });

this.demoDownlink.open();

When I open this downlink and see the Network tab..below 4 message come (which is all fine).
@sync(node:"demo_node_uri",lane:"demo_lane_uri")
@linked(node:"demo_node_uri",lane:"demo_lane_uri")
@event(node:"demo_node_uri",lane:"demo_lane_uri")["......"]
@synced(node:"demo_node_uri",lane:"demo_lane_uri")

When I try to close the downlink with the command this.demoDownlink.close();. Then below message is sent.
@Unlink(node:"demo_node_uri",lane:"demo_lane_uri")

But after sometime I still get the event message of the the closed downlink and it gets coming every time.
@event(node:"demo_node_uri",lane:"demo_lane_uri")["......"]

Also, when I try to again open the same laneUri, it only gives me the event message (and no sync, linked, event, synced).

Is there something I am missing? Can anyone give me an example on how to properly close the above downlink?

@brohitbrose
Copy link
Member

brohitbrose commented Jan 17, 2024

Hi @cyanaryan

The issue is that Swim consolidates the builder pattern for downlinks into the same class, which is convenient for the common-case (to avoid a separate builder class) but can lead to some awkward behavior. In your case, the "downlink" (really a "DownlinkBuilder" since it's incomplete) returned by:

this.demoDownlink = host
        .downlinkValue()
        .nodeUri('demo_node_uri')
        .laneUri('demo_lane_uri')
        .didSet((value) => {
          console.log(value.stringValue());      
        });

is actually separate from the downlink returned by the subsequent open() call. Only the first downlink, which is "incompletely built", is stored in your custom object's field, and closing it effectively does nothing (evidence for this is that you never saw an @unlinked response following your @unlink request).

Easiest thing to do is to do it all in one chained step:

this.demoDownlink = host
        .downlinkValue()
        .nodeUri('demo_node_uri')
        .laneUri('demo_lane_uri')
        .didSet((value) => {
          console.log(value.stringValue());      
        })
        .open();

If the open() must be a separate step, then reassign the field this.downlink to the result of open() later on.

I'll leave the issue open in case this doesn't address it, so please let me know if this worked for you.

@brohitbrose brohitbrose self-assigned this Jan 18, 2024
@brohitbrose brohitbrose added the C-question Category: question label Jan 18, 2024
@cyanaryan
Copy link
Author

Hi @brohitbrose

I tried this above solution. Actually, during some investigation, I find out that the problem is something else.

this.demoDownlink = host
        .downlinkValue()
        .nodeUri('/org/demo_node_uri/org')
        .laneUri('demo_lane_uri')
        .didSet((value) => {
          console.log(value.stringValue());      
        }).open();
        
 // Closing the downlink after use

this.demoDownlink.close();
  

Notice in the above nodeUri, it has a forward slash.

When I open this downlink, I get the following response
@sync(node:"/org/demo_node_uri/org",lane:"demo_lane_uri")
@linked(node:"/org/demo_node_uri/org",lane:"demo_lane_uri")
@event(node:"/org/demo_node_uri/org",lane:"demo_lane_uri")["......"]
@synced(node:"/org/demo_node_uri/org",lane:"demo_lane_uri")

I am getting the correct response. But when I try to close it. I get this below request
@Unlink(node:"org/demo_node_uri/org",lane:"demo_lane_uri")

Notice in the above request that, the forward slash is missing in here. I haven't done any changes in the uri. Is there any way on how to update the node_uri. Also why the forward slash is missing in here?

@brohitbrose
Copy link
Member

brohitbrose commented Jan 18, 2024 via email

@cyanaryan
Copy link
Author

Hi @brohitbrose

We are using "@swim/core": "4.0.0-dev.20210927.2",
I tried to switch to version "@swim/core": "4.0.0-dev.20230923",

But there are lots of imports error. I think there was major change when switching to above version.

@brohitbrose
Copy link
Member

@cyanaryan we may have to hack around it a bit since you're probably going to be unable to upgrade the version in your environment.

What is the value of the hostUri that you are connecting to? This information will tell us whether the bug is the one that we think it is and how we might need to proceed.

@cyanaryan
Copy link
Author

cyanaryan commented Jan 20, 2024

@brohitbrose
host uri value --> wss://demo-uri.labs.cloud/wss/

@brohitbrose
Copy link
Member

@cyanaryan In this early version of swim, having a path component in the hostUri (in your case the final wss/ is what is responsible for triggering the bug. Two possible solutions:

  • If you have control over the host address of the Swim server, see if you can change it to something with no path component

  • Otherwise we can help you patch your version, but this will be a bit complicated to coordinate

Please let us know which solution you'd like to proceed with.

@cyanaryan
Copy link
Author

cyanaryan commented Jan 22, 2024

@brohitbrose
So how should the hostUri look like? Shall we have to remove the slash at the end of the wss/
Can I make it like wss://demo-uri.labs.cloud/wss/com?

@brohitbrose
Copy link
Member

@cyanaryan Ideally the hostUri would have no path components (only protocol and domain and possibly a port at most), so in your case it would end in the .cloud (but any top-level domain like com or org will work here, if you're able to configure a new address to point to that location).

If you have this level of flexibility then this will be the quickest fix. Otherwise we will have to patch your version.

@cyanaryan
Copy link
Author

Hi @brohitbrose
I have started migrating to the latest version 4.0.0-dev.20230923 (previously using 4.0.0-dev.20210927.2).
I have few questions. regarding that.

  1. Is this the latest version, if yes then why the version says 4.0.0 (and also the dev tag) and why not 4.1.0 or 4.3?
  2. Is there any breaking changes because I am getting lots of imports error (like no TraitFastener, GenericTrait, etc)?

@ajay-gov
Copy link
Member

ajay-gov commented Feb 8, 2024

@cyanaryan

  1. 4.0.0-dev.20230923 is the latest version. We never released a 4.0.0 version, so this is going to be the 4.0.0 version.
  2. There are breaking changes unfortunately. Can you share the specific swim APIs and libraries you are using or maybe the compilation errors so that we can help you with it?

@cyanaryan
Copy link
Author

cyanaryan commented Feb 8, 2024

@ajay-gov
Okay, I'll go one by one.

import { GeoLayerController, GeoView, **GeoViewContext**, **GeoLayerView**, GeoRasterView, **GeoLineView**, GeoTreeView } from '@swim/maps';
import { GeoPoint } from '@swim/core';
import { Color, ControllerView, GraphicsIconView, ArcView, CircleIcon } from '@swim/ui';

export abstract class IconComponent extends GeoLayerController {
  // TODO: to check with swim on these defaults, if we can avoid
  @ControllerView<IconComponent, GeoRasterView>({
    type: GeoRasterView,
    key: true,
    observe: true,
    onSetView(newRasterView: GeoRasterView | null, oldRasterView: GeoRasterView | null): void {
      if (oldRasterView !== null) {
        this.owner.detachRasterView(oldRasterView);
      }
      if (newRasterView !== null) {
        this.owner.attachRasterView(newRasterView);
      }
    },
  })
  public declare raster: ControllerView<this, GeoRasterView>;

So the ControllerView has been removed from the @swim/ui. So what to use in alternative to that?

@ajay-gov
Copy link
Member

ajay-gov commented Feb 9, 2024

@cyanaryan Sorry about this. We will give you a working example of how to use maps with the new version. Will have it for you in a few days. Thanks

@cyanaryan
Copy link
Author

Sure @ajay-gov .
Please let me know once done. Thank you.

@cyanaryan
Copy link
Author

@ajay-gov

Any update regarding the working example?

@cyanaryan
Copy link
Author

Hi @ajay-gov

We are getting memory leak due to several downlinks being open. If possible, could you please update us about the documentation.

@cyanaryan
Copy link
Author

cyanaryan commented Apr 1, 2024

Hi @ajay-gov / @brohitbrose

We got the patched version file (got the swim-runtime zip file). In the mail response, it's mentioned that we need to update swim-core and swim-host in that. Do we have to update only the swim-core and swim-host to the version 4.0.0-dev.20230923?

Below is the mail response from the swim team

This is a patched version, requires an updated swim-core and swim-host js. Here it is bundled as swim-runtime.zip (includes a unminified version, minified version with the sourcemap as well). Can you give it to your team and ask them to try to see if it fixes the issue. If it does then we can figure out how to package and publish based on your team's needs?

Process I followed:
I removed the file that was there in the @swim/runtime folder (in dist/main basically) and updated it with the one given by the swim team. I ran the application but still the problem persist. I haven't updated swim-host and swim-core. They are on the version "4.0.0-dev.20210927.2. Below is the structure of the downlink we are calling

const host = client.hostRef(this.baseUrl);
this.downlink= host
       .downlinkValue()
      .nodeUri("/unit/master")
      .laneUri("latest")
       .didSet((value) => {
        
        });

HOST uri value ---> wss://demo-swim.os.cloud/wss/

PS: in the mail attachment we got 4 files
swim-runtime.zip

@ajay-gov
Copy link
Member

ajay-gov commented Apr 1, 2024

@cyanaryan yes please replace the swim-core and swim-host js files with the swim-runtime js file that is in the zip package. Let us know if that fixes the issue that you are having.

@cyanaryan
Copy link
Author

@ajay-gov
But while updating swim-host and swim-core (to the version 4.0.0-dev.20230923) , there are many errors that are coming. I was informed by the team lead that we don't have to change any code.

@ajay-gov
Copy link
Member

ajay-gov commented Apr 1, 2024

The version we gave you in the zip file is not 4.0.0-dev.20230923. It is a patch version, it has the fix to send the right unlink message so that it stops the data flow once you close the downlink. This patch version is built on top of 4.0.0-dev.20210927.2. So please use that version and not the new 4.0.0-dev.20230923 version

@cyanaryan
Copy link
Author

So I replaced the js file present in the swim-core and swim-host, with the content present in swim-runtime folder. Still the downlink are not getting closed

Steps I followed:

  1. Did npm i
  2. Removed the file present in the swim/runtime/dist/main folder and replaced it with file present in zip folder provided by the swim team.
  3. Removed the content present inside the swim/core/dist/main and replaced it with content present in swim-runtime folder.
    For example, the content present in swim-runtime.js ,swim-runtime.js.map ,etc has been copy pasted in the swim-core.js, swim-core.js.map, etc respectively.
  4. Did the same step for swim/host js file.
  5. Did npm start

PFA the screenshot

image

@ajay-gov
Copy link
Member

ajay-gov commented Apr 1, 2024

The fix involves changes to two files.

  1. UriPath.ts: The unmerge method there is differrent. It should be:
  unmerge(that: UriPath): UriPath {
    let base: UriPath = this;
    let relative = that;
    if (base.isEmpty()) {
      return relative
    }
    do {
      if (base.isEmpty()) {
        if (relative.isEmpty() || relative.tail().isEmpty()) {
          return relative;
        }
        return relative.tail();
      } else if (base.isRelative()) {
        return relative;
      } else if (relative.isRelative()) {
        return relative.prependedSlash();
      } else {
        let a = base.tail();
        let b = relative.tail();
        if (!a.isEmpty() && b.isEmpty()) {
          return UriPath.slash();
        } else if (a.isEmpty() || b.isEmpty() || a.head() !== b.head()) {
          return b;
        } else {
          a = a.tail();
          b = b.tail();
          if (!a.isEmpty() && b.isEmpty()) {
            return that;
          } else {
            base = a;
            relative = b;
          }
        }
      }
    } while (true);
  }
  1. RemoteHost.ts: Line 50 should be this.uriCache = new UriCache(hostUri.endpoint()); and it used to be this.uriCache = new UriCache(hostUri);

Can you please check to see you have those changes in the js file you are using to test the fix?

We can confirm that this works on our end. If you have those changes and it still doesn't work then let us get on a call to sort this out.

@cyanaryan
Copy link
Author

@ajay-gov

I updated UriPath.ts and RemoteHost.ts. I ran the application but still the downlink close issue persist. I even changed their js files too (UriPath.js and RemoteHost.js), but it still didn't work out.

If possible could we connect over call to resolve it. Let me know when you will be available.

@ajay-gov
Copy link
Member

ajay-gov commented Apr 2, 2024

@cyanaryan can you send me an email with your email address, we can coordinate over email My email is: ajay@nstream.io

@cyanaryan
Copy link
Author

Issue is fixed. Closing the bug. Swim is upgraded to 4.0.0-dev.20210927.3 (patched version) which has fix.
Thanks all for the help.

ajay-gov added a commit that referenced this issue Apr 6, 2024
…olkit (includes patch of UriPath and RemoteHost that addresses issue #156)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question Category: question
Projects
None yet
Development

No branches or pull requests

3 participants