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

Singleton and Member cache cleanup #79

Merged
merged 32 commits into from
Apr 16, 2013

Conversation

boudewijn-tribler
Copy link
Contributor

All Singleton methods are no longer available:

  • dispersy.has_instance
  • dispersy.get_instance
  • dispersy.del_instance

The Dispersy instance is available directly from the LaunchManyCore,
or though the session instance (session.lm.dispersy).

Also, DummyMember and Member instances should only be created through
Dispersy using:

  • dispersy.get_temorary_member_from_id (returns a DummyMember)
  • dispersy.get_new_member
  • dispersy.get_member
  • dispersy.get_members_from_id
  • dispersy.get_member_from_database_id

All Singleton methods are no longer available:
- dispersy.has_instance
- dispersy.get_instance
- dispersy.del_instance

The Dispersy instance is available directly from the LaunchManyCore,
or though the session instance (session.lm.dispersy).

Also, DummyMember and Member instances should only be created through
Dispersy using:
- dispersy.get_temorary_member_from_id (returns a DummyMember)
- dispersy.get_new_member
- dispersy.get_member
- dispersy.get_members_from_id
- dispersy.get_member_from_database_id
- dispersy_database -> dispersy.database
- join_community(master, ...) -> join_community(dispersy, master, ...)
- DummyMember(cid) -> dispersy.get_temporary_member_from_id(cid)
The origional change should not have been committed in the first place.
@boudewijn-tribler
Copy link
Contributor Author

There is still some work that needs to be discussed. Namely, where the callback should start/stop.

Up until now the callback was created and started by the LaunchManyCore. Hence, the callback should also be stopped by the LaunchManyCore. If we want, we can move both the start and stop to Dispersy. The same thing goes for the Endpoint, this is currently created in the LaunchManyCore and passed to Dispersy as a parameter.

Whatever we decide, we need to take into account that Dispersy is also started from dispersy/tool/main.py (and possibly other locations). When running from main.py dispersy should not call callback.start/stop since this would create a new thread, but instead run callback.loop on the main thread. Finally, StandaloneEndpoint has a start/stop that is called from main.py.

@NielsZeilemaker
Copy link
Contributor

I'm trying to reply to the comment of Boudewijn.
I agree, if the Launchmanycore starts the callbackthread, it should stop it.
Also calling dispersy.stop in order to clean some stuffs should be handled by launchmanycore.

@boudewijn-tribler
Copy link
Contributor Author

After a chat with Niels we agree on the following:

  • we will create Callback and Endpoint outside of Dispersy and pass them as parameters
  • we will add a start method to Dispersy
  • Dispersy.start will...
    1. start all registered communities (previously done in LaunchManyCore),
    2. start Callback, and
    3. start Endpoint.

Hence, we need to add start and stop methods to all Endpoint classes, even though only StandaloneEnpoint actualy uses it, and we will need to add a NoThreadCallback class that doesn't start a thread to use within dispersy/tool/main.py.

@boudewijn-tribler
Copy link
Contributor Author

I am not seeing any channels so something is broken, unfortunately I am not getting any errors. I had the same thing a few days ago, and Egbert could see errors...

@boudewijn-tribler
Copy link
Contributor Author

I have seen each test in http://jenkins.tribler.org/jenkins/job/Test_tribler_full-ui-run_stop working on my own PC at least once. test_libtorrent_download.py:test_stopresumedelete seems the most error prone, perhaps because it relies on external resources such as torrent files and peers. However, this test inconsistency is unrelated to this pull request.

I see no more issues holding this merge back. Please comment if you feel differently.

@NielsZeilemaker
Copy link
Contributor

The only strange thing I see is that the dht cannot bind on the socket.
It's reporting that its in use.
On which port is Dispersy binding? Is that causing a conflict?

2013/4/15 Boudewijn notifications@github.com

I have seen each test in
http://jenkins.tribler.org/jenkins/job/Test_tribler_full-ui-run_stopworking on my own PC at least once.
test_libtorrent_download.py:test_stopresumedelete seems the most error
prone, perhaps because it relies on external resources such as torrent
files and peers. However, this test inconsistency is unrelated to this pull
request.

I see no more issues holding this merge back. Please comment if you feel
differently.


Reply to this email directly or view it on GitHubhttps://github.com//pull/79#issuecomment-16382456
.

@boudewijn-tribler
Copy link
Contributor Author

lmc: Dispersy is listening on port 7759

As far as I know the Dispersy port number hasn't changed for a long time. But I recall that a relatively recent change to the DHT port number used the default port - 1... and DEFAULTPORT=7760.

I'm trying to find the change to confirm...

@boudewijn-tribler
Copy link
Contributor Author

Confirmed. It wasn't a problem before because Dispersy would add 1 to the port and try again, but since we now call Dispersy start earlier it is becoming the dht's problem. And the dht can't handle it.

Ive change it to using port 7757 (7758 is already taken by swift)

@NielsZeilemaker
Copy link
Contributor

Great, maybe run the ui tests again to check if the problem is resolved.

2013/4/15 Boudewijn notifications@github.com

Confirmed. It wasn't a problem before because Dispersy would add 1 to the
port and try again, but since we now call Dispersy start earlier it is
becoming the dht's problem. And the dht can't handle it.

Ive change it to using port 7757 (7758 is already taken by swift)


Reply to this email directly or view it on GitHubhttps://github.com//pull/79#issuecomment-16385240
.

@boudewijn-tribler
Copy link
Contributor Author

No, didn't resolve the existing issue. I suggest we attempt to solve below bug in its own issue, since this is unrelated to this pull request.

ERROR: test_downloadfrommagnet (Tribler.Test.test_libtorrent_download.TestLibtorrentDownload)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/workspace/Test_tribler_full-ui-run_stop/tribler/Tribler/Test/test_gui_as_server.py", line 153, in tearDown
    assert boolean, reason
AssertionError: no download progress

The delInstance of the superclass was called.  And since delInstance
removed cls.__single, this would result in a completely different
member since __single is renamed into __CLASSNAME_single.
@boudewijn-tribler
Copy link
Contributor Author

I've fixed a bug in the cache db, see boudewijn-tribler@0a3b2d4. The database singleton was never removed/closed at tribler shutdown, and the testcases kept using the same database cursor over and over. This solves an exception that occurs when the final call to commitNow doesn't cause a commit because there is nothing too commit.

Anyway. The test AssertionError: no download progress still occurs every now and then. It works just fine (sometimes) on my laptop, and it consistently fails running on Jenkins.

@NielsZeilemaker
Copy link
Contributor

Gui test seems to be working now, I'll ask Egbert to look into the
duplicate download exception.
Niels

2013/4/16 Boudewijn notifications@github.com

I've fixed a bug in the cache db, see boudewijn-tribler/tribler@0a3b2d4boudewijn-tribler@0a3b2d4.
The database singleton was never removed/closed at tribler shutdown, and
the testcases kept using the same database cursor over and over. This
solves an exception that occurs when the final call to commitNow doesn't
cause a commit because there is nothing too commit.

Anyway. The test AssertionError: no download progress still occurs every
now and then. It works just fine (sometimes) on my laptop, and it
consistently fails running on Jenkins.


Reply to this email directly or view it on GitHubhttps://github.com//pull/79#issuecomment-16434693
.

@boudewijn-tribler
Copy link
Contributor Author

Unless anyone feels the duplicate download is related to this pull request, I suggest we discuss improvement/cleanup to sqlitecachedb.py on a different issue. Also, performing this merge will allow us to continue with the dprint modifications without causes tons of merge problems.

@NielsZeilemaker
Copy link
Contributor

It's not, looking at this (
http://jenkins.tribler.org/jenkins/job/Test_tribler_full-ui-run_stop/18/artifact/tribler/output//Screenshot-09.png)
screenshot we can see that libtorrent is unable to fetch the .torrent from
the dht.
A clear libtorrent issue.

2013/4/16 Boudewijn notifications@github.com

Unless anyone feels the duplicate download is related to this pull
request, I suggest we discuss improvement/cleanup to sqlitecachedb.py on a
different issue. Also, performing this merge will allow us to continue with
the dprint modifications without causes tons of merge problems.


Reply to this email directly or view it on GitHubhttps://github.com//pull/79#issuecomment-16435445
.

@@ -1,6 +1,6 @@
[submodule "Tribler/dispersy"]
path = Tribler/dispersy
url = https://github.com/Tribler/dispersy.git
url = https://github.com/boudewijn-tribler/dispersy.git
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here, we are going to do both Dispersy and Tribler merge at the same time, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, you are right. but when I remove it the Jenkins task will point to the wrong repro. So it should be changed just before the merge.

Please enlighten me if there was another (better) way to do this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of any, right now :)

boudewijn-tribler added a commit that referenced this pull request Apr 16, 2013
Singleton and Member cache cleanup
@boudewijn-tribler boudewijn-tribler merged commit 183380a into Tribler:devel Apr 16, 2013
@boudewijn-tribler boudewijn-tribler deleted the dispersystop branch April 16, 2013 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants