swimos / swim

Full stack application platform for building stateful microservices, streaming APIs, and real-time UIs
https://www.swimos.org
Apache License 2.0
477 stars 41 forks source link

Data flow persists even after closing the downlink #156

Closed cyanaryan closed 3 months ago

cyanaryan commented 6 months ago

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 commented 6 months ago

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.

cyanaryan commented 6 months ago

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 commented 6 months ago

Hi @cyanaryan

This sounds like a bug in certain versions of swim-js that would take place if the hostUri is structured a certain way (most usually, but not necessarily, when there is some overlap with the nodeUri). The latest versions should not have this issue but I understand that upgrading may be incompatible with the Javascript version in your environment.

Can you confirm which version you are using and what value your hostUri takes?

On Thu, Jan 18, 2024 at 3:28 AM cyanaryan @.***> wrote:

Hi @brohitbrose https://github.com/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 https://github.com/sync (node:"/org/demo_node_uri/org",lane:"demo_lane_uri") @linked https://github.com/linked (node:"/org/demo_node_uri/org",lane:"demo_lane_uri") @event https://github.com/event (node:"/org/demo_node_uri/org",lane:"demo_lane_uri")["......"] @synced https://github.com/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 https://github.com/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?

— Reply to this email directly, view it on GitHub https://github.com/swimos/swim/issues/156#issuecomment-1898301922, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQKTJDILRUP4IS3VRSON5TYPEBPRAVCNFSM6AAAAABB6SWZ5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJYGMYDCOJSGI . You are receiving this because you were mentioned.Message ID: @.***>

cyanaryan commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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

brohitbrose commented 6 months ago

@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:

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

cyanaryan commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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

cyanaryan commented 4 months ago

@ajay-gov

Any update regarding the working example?

cyanaryan commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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);
    }
  2. 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 commented 3 months ago

@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.jsand 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 commented 3 months ago

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

cyanaryan commented 3 months ago

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.