saros-project/saros

Saros Plugin Development

Stefan Moll

@stefaus

@chrismotian If remote is ok, I'm available

Christian Zygmunt Jeschke

@chrismotian

I can't running (and writing) stf tests like https://github.com/saros-project/saros/blob/master/eclipse/test/stf/saros/stf/test/session/DerivedResourcesTest.java by simple click on "run Junit-Test" in Eclipse. Which set up is necessary? Is there available any help/manual ?

Stefan Moll

@stefaus

Here is a guide: https://www.saros-project.org/node/140

Kelvin Glaß

@m273d15

If the new guide is not sufficient, please add the missing information from the old documentation :wink: .

David Schreiber-Ranner

@davidschreiber

Hey there 👋So I submitted my first PR to the Saros project and also addressed feedback of the initial review round. (Thanks for providing the feedback that quick!) Now I wondered about the expected commit message style for those commits where I address review remarks. Since the PR is probably squashed upon merging (is this the case?) the commit message tags (INTERNAL, DOCS, etc.) are probably not all that relevant. What's common here? Sticking to the format just for aiding during review, or is this not defined that exact?

Here's the PR as reference: saros-project/saros#482

Stefan Moll

@stefaus

Hello David, welcome and thanks for your first PR!
Since the transition to github is more or less in progress, the new guidelines page is not yet actively published, but contains information about the tags used for commit messages and the review process description is here.
So the guidelines state that every commit message should be formatted accordingly, but this is a child of the Gerrit Review era when squash was not used. As you suspected, your commits are candidates for squashing and since your messages are meaningful enough for review, it looks good for me.

David Schreiber-Ranner

@davidschreiber

Alright! Thanks for confirming, and for pointing me to the guidelines 👌

Christian Zygmunt Jeschke

@chrismotian

Hey, is somebody available to take a look at #212. For unknown reason the build is queued forever. I need a review, too.

Christian Zygmunt Jeschke

@chrismotian

I stop work on project Saros. The concept of the Git Support is written in my thesis. From now on I don't have more time to invest and because of that I closed all my Issues and PR's.

Tobias Bouschen

@tobous

Is there any consensus on whether we want to be able to handle dropped network activities? (e.g. an activity is correctly send by one participant but never received by the others). @stefaus do you know which kind of network protocols we are using and whether they have any protection against package loss? I am asking because package loss is still a case that is not correctly handled by the new vector time reset logic

Tobias Bouschen

@tobous

From my current point of view, it is the only issue (handling the loss of deletion activities or ACKs) holding back the PR saros-project/saros#714 from my side (besides a minor outstanding adjustment filtering out certain checksum activities)

Stefan Moll

@stefaus

From skimming the code: ActivitySequencer is adding sequence numbers and checks them executeActivities(...).

As for the direct connection, TCP is used, so package loss is handled low level and retransmissions are tried.
For IBB Connections depends on the error type, if network drops, than again its TCP Handled and again retransmissions are tried.
If the Server drops single messages, than ActivitySequencer should handle this and simply drops the user.

Stefan Rossbach

@srossbach

There is a Ping Component, you will receive a user left at some point. The shutdown of a connection is still abrupt i.e you might miss the last package which is normally the leave package.

Tobias Bouschen

@tobous

@stefaus @m273d15 Time to test saros-project/saros#714 in a multi-user session? I created the branch https://github.com/saros-project/saros/tree/test_branch_714 for this purpose. It just add some logging and artificially increases the log level to make the important ones easier to spot.

Michael Schäfer

@mschaefer88

@tobous I should be able to join. Checked out your branch and ran runIde command with gradle.

Tobias Bouschen

@tobous

Ok, nice. Saros/I currently only supports 2 participants, so we are going to test on Saros/E

Stefan Moll

@stefaus

@tobous yes, did the same, so lets start a skype session in some minutes

Michael Schäfer

@mschaefer88

Can you help me out how I compile and start it in Eclipse? Or will the installed one be sufficient?

Stefan Rossbach

@srossbach

There is documentation on how to compile for Eclipse. Just make sure you run the gradle script with prepareEclipse option. Afterwards you can compile the project through the Eclipse IDE. Starting Tobias modification it simple. Checkout the patch, compile, and select a run configuration on simply launch it. A second Eclipse instance will open in which the plugin is already running.

@tobous I can join the session if you like. Please send me an email with the specific time.

Kelvin Glaß

@m273d15

We had a session with tobias and found some issues. Tobias will analyse the logs and create GH issues.

Tobias Bouschen

@tobous

@srossbach Yeah, you just missed it :wink: But we didn't really get far testing the patch as we found other issues regarding deleting files in Saros/E. When other users are actively typing in an editor while its file is deleted, it can happen that the editor stays open after the file was deleted on disk/from the workspace (even for the user that deleted the file in the first place). The issue seems to be that the editor is in a modified state when the file is deleted. But I haven't had time to investigate this further.

Tobias Bouschen

@tobous

And, for 3 person sessions, we had a weird issue that the old content or re-created files was sometimes still present for other participants (non-hosts that didn't cause the deletion). But this might be caused by my patch (which enables file re-creation in the first place)

Tobias Bouschen

@tobous

Actually, now that I think about it, the first issue should theoretically also be fixed by the activity filter introduced with the vector time fix if the issue is really edit activities received after the deletion. If, however, the issue is the modified state of the editor on deletion, this would not be the case. We moved away from testing the vector time patch relatively quickly as we wanted to make sure that the issues couldn't be reproduced on the master. I am not sure whether the editor issue was present with the vector time patch. Either way, I will have a closer look at the logs. @srossbach if you are interested, I can also create an issue for it later this evening and upload the logs so that you can have a look at it as well.

Stefan Rossbach

@srossbach

@tobous when you delete a file, does the editor sometime stay open or all the time ? As far as I know we do not close any editor stuff in Eclipse when we receive a file activity. So the editor should always stay open.

Tobias Bouschen

@tobous

Well, normally, the editor also gets closed automatically. This seems to be dependent on the editor state as far as I can tell. My guess would be that the editor doesn't get closed automatically when the local editor is in an unsaved state (maybe to avoid information loss?).
This is important for the consistency check, as it is based on open editors. During our tests, the editor did not stay open for all participants (maybe only for the host?), leading to inconsistency state triggers for other participants whose editor was correctly closed. This inconsistency was triggered even though the content on disk should have been consistent (as the file was also no longer present on disk on the host's side as well).

Maybe an explicit handling of the editor would fix the issue. In Saros/I, the logic closes the editor, disables the save listener (to ensure that no unnecessary activities are created), saves the file to disk and then deletes the file (and re-enables the listeners). This ensures that there aren't any inconsistent states between the representation of the file on disk and as a "document" (loaded content modifiable in the editor).

Tobias Bouschen

@tobous

Also, Eclipse seems to try to handle this. It warns the user when an unsaved file is deleted. Maybe this warning causes the editor to be saved to avoid such issues. Of course, this would not work when the editor is edited in parallel by incoming activities. Is there any blocking logic or can such a parallel access (i.e. an editor is modified while the local deletion triggered by the user is processed) happen? The warning dialog does not appear to be modal (i.e. the Eclipse seems to run unhindered in the background while the window is open), so maybe that is an issue as well.

Stefan Rossbach

@srossbach

We execute any activity regardless if there is a modal dialog present or not as Eclipse has no support for this and I am will not install my own synchronizer into the SWT display. The whole Eclipse stuff is a little bit more complicated. Especially the change on how the content for editors is retrieved has broken many things regarding the watchdog. The problem is that we hold a reference for each user on the buffer of the editor. If these references are not cleared, i.e no editor close event is send, the content of the editor is still present on the host although the file that belong to the editor is already deleted. Last time I did a test the watchdog did not detect anything although this was working at some point. Open a file. Modify it and then set the remote user to read-only and close the file. You will not get an inconsistency warning. Even if you reopen the file on the host you still will not get an inconsistency warning although you can see that the content did not match. Only after you insert at least 1 character an inconsistency is detected.

Tobias Bouschen

@tobous

While having a look at the log files, I notices that the log files for Saros/E are still ASCII encoded, while log files for Saros/I use UTF8. I only notices this as they displayed umlaut characters as their encoding instead of the character itself (i.e. 'ö' as '\u00F6'), so this isn't really an issue, it just seems weird to me.
Is there a reason to use ASCII (besides the reduced file size)?

Tobias Bouschen

@tobous

@Flowdalic Uff, its been a while. I think the smack stuff just got lost in the chaos. :sweat_smile: But we are still very much interested in updating smack. We have been working hard on updating our outdated libraries along with our build infrastructure and smack is one of the big ones, especially since it provides a central functionality of the plugin and is in a somewhat messy state with all of the patch stuff we added. So we really appreciate your efforts.

I had a short look at your branch but didn't really get far. (But I am also not that well-versed in the network part of Saros, so that doesn't really mean that much :wink:) As @srossbach already mentioned, formatting the code with our formatter (google java format 1.6) reduces the PR size considerably (from +3300 -4800 LOC to +1000 LOC - 2700 LOC, which is still a lot, but more manageable).

The patch seems like a WIP (judging from the compiler errors). Are you still interested in working on the migration? If so, what are your general plans? Maybe we can work out some sort of cooperation to make the migration easier. :smile: Do you have a general estimate on how much work still is necessary to complete the migration?

Mehmet Cetinkaya

@Memo2109

Extending User Permissions

My job is to come up with an extension of the User Permissions mechanism. I've seen there is already a somewhat flexible Permission implemented in the User Class:

https://github.com/saros-project/saros/blob/master/core/src/saros/session/User.java#L35

My question: What would you like specifically to be extended? Are there use cases for currently missing functionality? The only problem i could find until now is the missing Permission for Users to upload documents into the Session. From my point of view it would be sufficient to add a specifier to the enum Permissions and an instance method like hasShareDocumentPermission()..?

Kelvin Glaß

@m273d15

Hi, @stefaus, @Drakulix and me already talked a bit about your topic (Adding a permission model that will improve the server workflow). I'll post a summary of our talk below. @Memo2109 I think you should concentrate on the first step mentioned in this summary. @srossbach what do you think?

First Step: Adding a permission model in Saros

The current server just accepts all requests of users to join the session.
Therefore it is necessary to think about a permission model.

Permissions

join a session
share a new project in the session
grant permissions to other users

Implementation

Using different permission models for server sessions and common session (without a server) is unnecessary. Therefore, it is necessary to create a permission model that allows to implement the current user permissions (the permissions of the host user and other users).

Second-Step: Super-server with permission model

What are the tasks which are provided by a super-server

Start a session server
Configure a session server (might be not necessary)
Stop a session server
(Currently not in the scope: session persistence)

Permissions

Grant permissions to other users
Start a session server
Stop a session server
Configure server

Implementation

Proposed by @stefaus: The super server has one JID (e.g. super-server@jabberd.org)
All session servers are sharing one JID (e.g. server@jabberd.org) and are identified by an individual resource.

Initial super-server admin and grant mechanism:
The initial super-server admin is just configured in a configuration file of the super-server. All further admins are added via communication between an admin and the super-server.
Who receives the initial admin permissions (all permission on session level) of a session?
- The first user in the session?
- Configured before starting the session by the user with the permissions to create a new session in the super-server?
Who receives the admin permissions of a session if the last user with admin permissions left the session?
- The next one in the session list (who is the next one?)?
- All users become session admins?
- There is no admin anymore (session is not very usable anymore as without a server)?

Stefan Rossbach

@srossbach

There are also other security concerns. Who is able to invite someone to a session. What about protecting a session with a password ? Lets say you have a session with Alice, Bob, Carl. While everybody has write access, only Alice is allowed to invite another user to a session that does not have the password. However if Alice leave the session it has to be decided what (if you want to keep the session alive) the next "host" permissions are. Maybe it will inherit all permissions from the former host or maybe not.

Mehmet Cetinkaya

@Memo2109

Hi, @all, @m273d15 ,

Hi @all,
I just put the new new privileges/permissions for session users online and wanted to show you my design and I also tried to implement a Junit-test. I would be happy to receive your feedback. Please let me know what you think.

Mehmet Cetinkaya

@Memo2109

https://github.com/saros-project/saros/compare/master...Memo2109:core/permissions/addUserPermissions?expand=1

Stefan Moll

@stefaus

@Memo2109 Hey Mehmet, normaly please create a (draft) pull request to get feedback for your code.

I think instead of creating a separate UserPrivilege class, using a EnumSet would be a simpler approach. What you have to keep in mind is also concurrency. Using a HashMap will not work here (neither EnumSet without additional handling) and instead of a separate method to check Privileges, this should stick to a single get method like your hasPrivilege.
About the Privileges, a User is created inside a Session, thats why handling Permissions like to Start a Session Server here will not work.

Mehmet Cetinkaya

@Memo2109

@stefaus Hey Stefan, thank you very much for your feedback

Kelvin Glaß

@m273d15

Hi, @tobous proposed to write meeting minutes in stand-ups etc. Therefore, I created a corresponding channel

that should be used only for the minutes. The channel is currently public. If we need a more strict policy in the future we can change it. Please, discuss topics (which are mentioned in the meeting-minutes) in this channel. Otherwise the new channel becomes crowded.

Tobias Bouschen

@tobous

Once saros-project/saros#752 is merged, I would like to do another round of testing for the vector time patch saros-project/saros#714. As I was not able to produce any issues in a two person setup (even with concurrent activities), a multi-user test is quite important (especially with the weird issues that occurred the last time).

@m273d15 @stefaus @srossbach What does your availability look like in next week? My initial suggestion would be Monday or Tuesday afternoon, but I don't have any set events/meetings on Monday, Tuesday, or Thursday, so I am relatively open timewise. Wednesday before 17:00 would be fine as well.

Mehmet Cetinkaya

@Memo2109

I have one question, as I make my last PR I have seen that tests startet like codacy and travis ci. My question is should I also make the codacy and travis ci tests local on my repo befor I make a PR ? If yes have you an introduction for setting up codacy and travis ci. Thank you very much.

Tobias Bouschen

@tobous

I have one question, as I make my last PR I have seen that tests startet like codacy and travis ci. My question is should I also make the codacy and travis ci tests local on my repo befor I make a PR ? If yes have you an introduction for setting up codacy and travis ci. Thank you very much.

@Memo2109 No, you don't have to add the checks to your local fork of the repository. As you already pointed out, they are always run when you create a PR onto the main repo.

Tobias Bouschen

@tobous

I changed the description of the Area: X issue labels to state "Issues affecting X". In my opinion, this makes the tags much more usable as they then provide a good tool for getting an overview which PRs affect which area of the code. In my opinion, the current restrictions of having them be "specific to the area" makes sense for issues but not for PRs, as they often bundle multiple commit dealing with different parts of an issue.

Anyone have any issues with this change?

Kelvin Glaß

@m273d15

Anyone have any issues with this change?

No :+1:

Kelvin Glaß

@m273d15

I wondered what is currently missing in order to release a new Saros/Eclipse version. The corresponding project contains just one small issue. Is this the correct state? What do you want to see merged/fixed/changed before the next Saros/Eclipse release?

Tobias Bouschen

@tobous

I would definitely still like to get saros-project/saros#714 merged before the next release to finally fix the vector time issue. I didn't add it to the project as it doesn't specifically deal with a Saros/E issue but rather with an issue in the core.

Kelvin Glaß

@m273d15

As discussed in the last stand-up (see meeting minutes) we (me and @tobous @stefaus ) want to make a Skype meeting to discuss the design and separation of Core and UI logic. Who is interested (@srossbach ? )?
What do you think about 12.12 at 16:30 (I am flexible, we can change the date/time)?

Tobias Bouschen

@tobous

12.12. at 16:30 isn't ideal for me. I have a meeting at 15:15 for which I don't know how long it is going to take. I could be done by 15:45 but it could easily also take until 17:00 (or longer), depending on my waiting time.

For me, the following dates would work better:

the 9.12. (any time)
the 10.12. (any time)
the 11.12. before 17:00
the 12.12. before 14:30
the 13.12. (any time)

Or, if don't find any other date/time, you could keep the session on the 12.12. at 16:30 (or 17:00) and I will join in as soon as I can.

Where communities thrive

People

Repo info

Activity

First Step: Adding a permission model in Saros

Permissions

Implementation

Second-Step: Super-server with permission model

What are the tasks which are provided by a super-server

Permissions

Implementation