Skip to content

Conversation

@alperozturk96
Copy link
Contributor

@alperozturk96 alperozturk96 commented Nov 27, 2024

The client must be able to modify the session timeout according to their needs. For example, if the server instance slow, some network operations may fail. This could lead to issues, especially during end-to-end (E2E) operations.

Changes

SessionTimeOut data class has been added.
The default SessionTimeOut is utilized for each remote operation in the first constructor.
A second constructor has been added to allow custom SessionTimeOut injection for each remote operation.

How to Test?

Files client must build without any compile error
Files client must be able to configure SessionTimeOut for any remote operations

@github-actions
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3434
Dodgy code2626
Internationalization66
Malicious code vulnerability4949
Multithreaded correctness33
Performance88
Total161161

Signed-off-by: alperozturk <[email protected]>
@github-actions
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3434
Dodgy code2626
Internationalization66
Malicious code vulnerability4949
Multithreaded correctness33
Performance88
Total161161

Signed-off-by: alperozturk <[email protected]>
@github-actions
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3434
Dodgy code2626
Internationalization66
Malicious code vulnerability4949
Multithreaded correctness33
Performance88
Total161161

@alperozturk96
Copy link
Contributor Author

Failing test is unrelated. Example merged PR: #1593

Copy link
Contributor

@ZetaTom ZetaTom left a comment

Choose a reason for hiding this comment

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

Overall, this seems to be working as expected. There are a few minor things that I would suggest changing, for which I've added some comments.

Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
@github-actions
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3434
Dodgy code2626
Internationalization66
Malicious code vulnerability4949
Multithreaded correctness33
Performance88
Total161161

Signed-off-by: alperozturk <[email protected]>
@github-actions
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3434
Dodgy code2626
Internationalization66
Malicious code vulnerability4949
Multithreaded correctness33
Performance88
Total161161

@nextcloud-android-bot
Copy link
Collaborator

@alperozturk96
Copy link
Contributor Author

@tobiasKaminsky @AndyScherzinger Can we merge this PR? Since unrelated tests are keep failing for previously merged PRs as well.

Copy link
Contributor

@ZetaTom ZetaTom left a comment

Choose a reason for hiding this comment

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

Apart from the new default timeouts, everything looks fine. I haven't noticed any change in behaviour when running tests. However, we need to consider whether the new timeouts have any unintended consequences.

@alperozturk96
Copy link
Contributor Author

alperozturk96 commented Dec 3, 2024

I think increasing SYNC_READ_TIMEOUT and SYNC_CONNECTION_TIMEOUT values should not cause any issues, and it is straightforward changes.

However, if there’s a argument against this, it would be great to discuss it further. Any code modification can sometimes lead to unexpected behavior :)

@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

@tobiasKaminsky tobiasKaminsky merged commit 3870fec into master Dec 9, 2024
16 checks passed
@tobiasKaminsky tobiasKaminsky deleted the add-optional-session-time-out-duration branch December 9, 2024 08:26
@alperozturk96
Copy link
Contributor Author

/backport to stable-2.19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants