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

component.stop() should not return session.leave() result (if session exists) #1075

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
4 participants
@npeditto
Copy link
Contributor

npeditto commented Nov 6, 2018

In component.py the stop() should do session.leave() and return the a future object if session exists (not return session.leave() result only).

Otherwise we are not able to call again the component.start(...) method after stop() method called.

in component.py the stop() should do session.leave() and return the f…
…uture (not return session.leave() result)
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #1075 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1075   +/-   ##
=======================================
  Coverage   41.55%   41.55%           
=======================================
  Files          69       69           
  Lines       13197    13197           
  Branches     2205     2205           
=======================================
  Hits         5484     5484           
  Misses       7293     7293           
  Partials      420      420
Impacted Files Coverage Δ
autobahn/wamp/component.py 38.93% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2645f1e...4ef9f1a. Read the comment docs.

@oberstet oberstet requested a review from meejah Nov 6, 2018

@oberstet

This comment has been minimized.

Copy link
Member

oberstet commented Nov 6, 2018

actually, looks good and merging (as I am pushing a release) --

@oberstet oberstet merged commit 3cd1083 into crossbario:master Nov 6, 2018

2 of 3 checks passed

codecov/patch 0% of diff hit (target 41.55%)
Details
codecov/project 41.55% remains the same compared to 2645f1e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

I don't understand what problem you're trying to solve here? session.leave does return a future/deferred (that fires when the session actually closes). If you don't care about that, just ignore the return value from .stop()

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

(If you do care about waiting for the disconnect to actually happen there won't be any way to do that now)

oberstet added a commit that referenced this pull request Nov 6, 2018

Revert "in component.py the stop() should do session.leave() and retu…
…rn the future (not return session.leave() result) (#1075)"

This reverts commit 3cd1083.

oberstet added a commit that referenced this pull request Nov 6, 2018

Revert "in component.py the stop() should do session.leave() and retu…
…rn the future (not return session.leave() result) (#1075)" (#1076)

This reverts commit 3cd1083.
@oberstet

This comment has been minimized.

Copy link
Member

oberstet commented Nov 6, 2018

@meejah uups, yes, right .. we do want to return the deferred from leave. lack of sleep, too much hurry;)

@npeditto

This comment has been minimized.

Copy link
Contributor

npeditto commented Nov 6, 2018

In a scenario where I call "component.start(loop)" after a "component.stop()" with autobahn v18.9.2 an application is able to reconnect. Through the latest version (18.10.1) where the component.py has been updated (in particular, _start() and stop() methods) this application is not able to reconnect to Crossbar.

In particular, the attribute "self._done_f" in "_start(...)" method is not reset to "None". I tried also another workaround: in stop() method I set "self._done_f" to "None" before the "return self._session.leave()". So this could be another solution.

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

Component already knows how to re-connect, you shouldn't have to call start() again (and _start is an internal method, you should only call .start() the public API). Can you show some example code about what you're trying to accomplish?

@npeditto

This comment has been minimized.

Copy link
Contributor

npeditto commented Nov 6, 2018

I don't understand what problem you're trying to solve here? session.leave does return a future/deferred (that fires when the session actually closes). If you don't care about that, just ignore the return value from .stop()

Yes, I was wrong about future object return. Maybe the problem, that I described in the previous post, is related to the "self._done_f" attribute that is not set to "None" in stop() method.

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

Maybe, I'm not sure I ever anticipated someone calling .start() more than once. What are you trying to do..?

@npeditto

This comment has been minimized.

Copy link
Contributor

npeditto commented Nov 6, 2018

Component already knows how to re-connect, you shouldn't have to call start() again (and _start is an internal method, you should only call .start() the public API). Can you show some example code about what you're trying to accomplish?

Yes of course. I used in my code the start() method (not _start()).

In autobahn 18.9.2 if I call
component.stop()
(because I need for my reasons to close the wamp connection)

and then call
component.start(loop)

my application was able to reconnect to Crossbar again.

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

(And yes, it's probably more-correct to reset _done_f to None at some point or at least make it an error to call .start() a second time if we really can't/won't support that ... but I do want to understand your use-case..)

So essentially you have a reason to want to disconnect your Component for some amount of time and then re-connect it..?

@npeditto

This comment has been minimized.

Copy link
Contributor

npeditto commented Nov 6, 2018

(And yes, it's probably more-correct to reset _done_f to None at some point or at least make it an error to call .start() a second time

It should be fine to reset _done_f to None

So essentially you have a reason to want to disconnect your Component for some amount of time and then re-connect it..?

Yes, I need to disconnect my application and reconnect it. Resetting the _done_f before return session.leave() I will be able to do that again (as in 18.9.2 version).
So for me is very important to avoid to "make it an error to call .start() a second time".

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

I think the correct fix is probably for _done_f to be reset after the leave() deferred fires. So your application code then might look something like:

my_component.start(..)
# ...
await my_component.stop()
# ...
my_component.start(..)
@npeditto

This comment has been minimized.

Copy link
Contributor

npeditto commented Nov 6, 2018

my_component.start(..)
# ...
await my_component.stop()
# ...
my_component.start(..)

Exactly that is my workflow.

I think the correct fix is probably for _done_f to be reset after the leave() deferred fires.

I agree! Should the _done_f reset be insert into the stop() method (in component.py)?

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

Probably what should be done is anywhere we .resolve() the _done_f it should be reset to None..? (but maybe only on _stopping). Might need more thought for the different cases.

@meejah

This comment has been minimized.

Copy link
Member

meejah commented Nov 6, 2018

Moving to a ticket: #1077

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 28, 2018

adam
py-autobahn: updated to 18.11.2
18.11.2:
fix: asyncio unregisterProducer raises exception
fix: URL is not required in RawSocket configuration items with WAMP component API
fix: revert crossbario/autobahn-python#1075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment