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.

Sign up for GitHub

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

Jump to bottom

Fix event subscription and improve Counter example #1008

Merged
merged 5 commits into from
Jun 9, 2023
Merged

Fix event subscription and improve Counter example #1008

merged 5 commits into from
Jun 9, 2023

Conversation

relu91
Copy link
Member

@relu91 relu91 commented May 19, 2023

This PR should eventually fix #1005 and might solve #973 as well. Moreover, I wanted to improve a little bit the UI in the counter-example to give users small feedback when events occur. Everything is still draft because I need to clean up the code, the commit log, and typescript typings. Other than that it should work and people can try it.

danielpeintner
danielpeintner reviewed May 22, 2023
View reviewed changes
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I checked it locally and it seems to work fine 👍.

Some questions:

  • changing online URL to local url (see comment below)?
  • for events: Once subscribed again it remembers the "old" payload while the event count is set back to zero: I think the payload should be also nulled. What do you think?
  • How can we add a test to avoid regressions in the future?

examples/browser/counter.html Outdated Show resolved Hide resolved
@relu91 relu91 force-pushed the fix_counter_example branch from d8a7bd1 to 18af484 Compare May 31, 2023 14:59
@relu91 relu91 marked this pull request as ready for review May 31, 2023 15:00
@relu91 relu91 requested a review from danielpeintner May 31, 2023 15:00
@relu91
Copy link
Member Author

relu91 commented May 31, 2023

I reviewed the updated code and I noticed that I left a couple of anys around. This is shameful 😸 . I'll fix asap. Btw I was reading old issues and I think this is relevant to #230. I think adding the abortController fixes the points that we were discussing. I just want to be sure if aborting the request in the close method is enough.

@relu91
Copy link
Member Author

relu91 commented May 31, 2023

About the any issue I find out that there were a bug in Typescript 4.4.3 until around 4.7.x. I'll try to update the typescript version. Any reason to not do that?

@danielpeintner
Copy link
Member

About the any issue I find out that there were a bug in Typescript 4.4.3 until around 4.7.x. I'll try to update the typescript version. Any reason to not do that?

I don't see any reason not to update 👍

danielpeintner
danielpeintner reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I think it looks good but I still have the questions below (as raised before)

  • for events: Once subscribed again it remembers the "old" payload while the event count is set back to zero: I think the payload should be also nulled. What do you think?
  • How can we add a test to avoid regressions in the future?

examples/browser/counter.js Show resolved Hide resolved
@relu91 relu91 force-pushed the fix_counter_example branch from db6c574 to 3eb59de Compare June 7, 2023 09:13
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023 •
edited
Loading

Codecov Report

Merging #1008 (26598be) into master (fa9b6f7) will increase coverage by 0.00%.
The diff coverage is 78.72%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #1008   +/-   ##
=======================================
  Coverage   75.79%   75.79%           
=======================================
  Files          72       72           
  Lines       14856    14871   +15     
  Branches     1429     1428    -1     
=======================================
+ Hits        11260    11272   +12     
- Misses       3563     3566    +3     
  Partials       33       33
Impacted Files Coverage Δ
packages/binding-http/src/http-server.ts 63.74% <0.00%> (-0.35%) ⬇️
packages/core/src/consumed-thing.ts 83.93% <96.96%> (+0.44%) ⬆️
...ackages/binding-http/src/subscription-protocols.ts 97.74% <100.00%> (+0.08%) ⬆️

@relu91
Copy link
Member Author

relu91 commented Jun 7, 2023

for events: Once subscribed again it remembers the "old" payload while the event count is set back to zero: I think the payload should be also nulled. What do you think?

I should revert back to an empty string here. Is that ok?

More info on #973. With the current changes the memory leak is fixed for the first subscription but the problem arise again if you follow these steps:

  • subscribe
  • generate a couple of events
  • unsubscribe
  • subscribe again
  • generate another series of events (more than 11)
  • Memory leak detected

I think this is due to the fact that here the server response is reused and the client keep it open. I have to investigate more.

@relu91
Copy link
Member Author

relu91 commented Jun 7, 2023 •
edited
Loading

How can we add a test to avoid regressions in the future?

It would be nice to have something similar to what respec has. They are using karma to run a couple of test suites directly on the browsers.

@relu91
fix(binding-http): long polling handling of unsubscriptions
26598be 
use `close` event instead of finish to detect unsubscriptions. Close event
is also called when the client abort the connection. `finish` is only called
when the client consume the response and close the connection.
@relu91
Copy link
Member Author

relu91 commented Jun 7, 2023

26598be fix #973

@relu91 relu91 requested a review from danielpeintner June 7, 2023 13:01
@egekorkan
Copy link
Member

How can we add a test to avoid regressions in the future?

It would be nice to have something similar to what respec has. They are using karma to run a couple of test suites directly on the browsers.

In playground, we use playwright to test the interface: https://github.com/thingweb/thingweb-playground/blob/master/packages/web/test.js . It works pretty well and has a quite simplistic way to program the tests. Nothing against karma, just one alternative.

@danielpeintner danielpeintner mentioned this pull request Jun 8, 2023
Open
@danielpeintner
Copy link
Member

I should revert back to an empty string here. Is that ok?

👍

@relu91 relu91 force-pushed the fix_counter_example branch from 0b92aa8 to b4d786f Compare June 9, 2023 08:09
danielpeintner
danielpeintner approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

LGTM

@danielpeintner danielpeintner merged commit ba3f932 into eclipse-thingweb:master Jun 9, 2023
@danielpeintner danielpeintner mentioned this pull request Jun 9, 2023
Closed
@relu91 relu91 mentioned this pull request Jun 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielpeintner danielpeintner danielpeintner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Events does not work with URIVariables
4 participants
@relu91 @danielpeintner @codecov-commenter @egekorkan